Skip to content

Conversation

@sarangsapre
Copy link

Fixing issue #19

@ghostoy
Copy link

ghostoy commented Jan 9, 2015

The build failure is caused by npm on older node.js. @TooTallNate Would you like to merge this?

@TooTallNate
Copy link
Owner

I don't really get why this is necessary honestly. Shouldn't nan be taking care of this for us? /cc @rvagg @kkoopa

@rvagg
Copy link

rvagg commented Jan 9, 2015

Off the top of my head (there there isn't very much), this is because of a change to Set for usage where you probably should be using ForceSet anyway. The ForceSet API is consistent so NAN doesn't need to wrap anything. I think this change makes sense and I don't believe there's a role for NAN here.

.. I don't recall the other project where this came up recently but it was the same thing and the same resolution.

@kkoopa
Copy link

kkoopa commented Jan 9, 2015

Yes, I remember something like that too, but not where it showed up. Maybe a closed issue in the nan repo?

On January 10, 2015 1:29:23 AM EET, Rod Vagg [email protected] wrote:

Off the top of my head (there there isn't very much), this is because
of a change to Set for usage where you probably should be using
ForceSet anyway. The ForceSet API is consistent so NAN doesn't need
to wrap anything. I think this change makes sense and I don't believe
there's a role for NAN here.

.. I don't recall the other project where this came up recently but it
was the same thing and the same resolution.


Reply to this email directly or view it on GitHub:
#20 (comment)

@kkoopa
Copy link

kkoopa commented Jan 18, 2015

It was in node-sqlite3 TryGhost/node-sqlite3#369
via NAN nodejs/nan#221

The answer seems to be ForceSet.

TooTallNate added a commit that referenced this pull request Jan 18, 2015
Using v8::Object::ForceSet instead of v8::Object:Set
@TooTallNate TooTallNate merged commit a12d883 into TooTallNate:master Jan 18, 2015
@TooTallNate
Copy link
Owner

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants