New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for NAN@2 (incomplete) #181

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@rvagg
Contributor

rvagg commented Sep 10, 2015

The beginings of a NAN@2 upgrade, not complete cause it segfaults, someone else is willing to take over this if they have the patience to work out why. It compiles at least.
#173

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 10, 2015

Contributor

I couldn't leave it alone, it works now but I've only tested on Node 4 so I have no idea if this plays nicely with older Node and I don't use contextify so ymmv.

Contributor

rvagg commented Sep 10, 2015

I couldn't leave it alone, it works now but I've only tested on Node 4 so I have no idea if this plays nicely with older Node and I don't use contextify so ymmv.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Sep 10, 2015

Adding to the list: nodejs/node#2798.

ChALkeR commented Sep 10, 2015

Adding to the list: nodejs/node#2798.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol commented Sep 10, 2015

Related: #180 (comment)

@mgol mgol referenced this pull request Sep 10, 2015

Closed

Upgrade NAN for Node 4.0.0 #180

@brianmcd

This comment has been minimized.

Show comment
Hide comment
@brianmcd

brianmcd Sep 11, 2015

Owner

Thank you so much, @rvagg! I'll do some regression testing on this later tonight and hopefully get a new version pushed out soon.

Owner

brianmcd commented Sep 11, 2015

Thank you so much, @rvagg! I'll do some regression testing on this later tonight and hopefully get a new version pushed out soon.

@reggi

This comment has been minimized.

Show comment
Hide comment
@reggi

reggi Sep 12, 2015

Really happy progress is being made to fix this. Thanks everyone.

I just tried to install @rvagg's version and it's not installing on node.js v4.0.0 here's my npm-debug.log, hopes this helps.

reggi commented Sep 12, 2015

Really happy progress is being made to fix this. Thanks everyone.

I just tried to install @rvagg's version and it's not installing on node.js v4.0.0 here's my npm-debug.log, hopes this helps.

@brianmcd

This comment has been minimized.

Show comment
Hide comment
@brianmcd

brianmcd Sep 13, 2015

Owner

@rvagg - Thanks again for your help on this. It works like a charm on 4.0.0, but I get the following errors on older versions:

Any chance you'd be able to help out there? I added travis to this repo with the tests running against node 4, 0.12, and 0.10, so it should make multi-version testing a little less painful. Another push here should trigger it.

Owner

brianmcd commented Sep 13, 2015

@rvagg - Thanks again for your help on this. It works like a charm on 4.0.0, but I get the following errors on older versions:

Any chance you'd be able to help out there? I added travis to this repo with the tests running against node 4, 0.12, and 0.10, so it should make multi-version testing a little less painful. Another push here should trigger it.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 13, 2015

Contributor

Sorry, I was using v8::MaybeLocal (implicit v8 namespace) which doesn't exist on those versions, switched to Nan::MaybeLocal. I've fixed that now and it mostly works with one remaining hiccup in the tests for v0.10.40. I've also added a .dntrc file just in case you're interested in testing with dnt.

$ dnt
node@v0.10.40: FAIL wrote output to /tmp/contextify-dnt-v0.10.40.out
node@v0.12.7 : PASS wrote output to /tmp/contextify-dnt-v0.12.7.out
node@v1.8.4  : PASS wrote output to /tmp/contextify-dnt-v1.8.4.out
node@v2.5.0  : PASS wrote output to /tmp/contextify-dnt-v2.5.0.out
node@v3.3.0  : PASS wrote output to /tmp/contextify-dnt-v3.3.0.out
node@v4.0.0  : PASS wrote output to /tmp/contextify-dnt-v4.0.0.out

@kkoopa I'm at a loss on this one, only failing on v0.10.40. It's failing on these:

        test.throws(function () {
            sandbox.run('doh');
        }, ReferenceError);
        test.throws(function () {
            sandbox.run('x = y');
        }, ReferenceError);

and when you inspect what it does, at line 98 in contextify.cc, this:

        Nan::MaybeLocal<Value> result = Nan::RunScript(script.ToLocalChecked());

is returning an undefined but is not empty and the TryCatch isn't catching anything. As far as I can tell it's doing exactly the same thing as it was with NAN@1 (via Handle<Value> result = NanRunScript(script);) but now it's not passing the error back up so I suppose it's to do with other infrastructure in place around it. Ideas?

Contributor

rvagg commented Sep 13, 2015

Sorry, I was using v8::MaybeLocal (implicit v8 namespace) which doesn't exist on those versions, switched to Nan::MaybeLocal. I've fixed that now and it mostly works with one remaining hiccup in the tests for v0.10.40. I've also added a .dntrc file just in case you're interested in testing with dnt.

$ dnt
node@v0.10.40: FAIL wrote output to /tmp/contextify-dnt-v0.10.40.out
node@v0.12.7 : PASS wrote output to /tmp/contextify-dnt-v0.12.7.out
node@v1.8.4  : PASS wrote output to /tmp/contextify-dnt-v1.8.4.out
node@v2.5.0  : PASS wrote output to /tmp/contextify-dnt-v2.5.0.out
node@v3.3.0  : PASS wrote output to /tmp/contextify-dnt-v3.3.0.out
node@v4.0.0  : PASS wrote output to /tmp/contextify-dnt-v4.0.0.out

@kkoopa I'm at a loss on this one, only failing on v0.10.40. It's failing on these:

        test.throws(function () {
            sandbox.run('doh');
        }, ReferenceError);
        test.throws(function () {
            sandbox.run('x = y');
        }, ReferenceError);

and when you inspect what it does, at line 98 in contextify.cc, this:

        Nan::MaybeLocal<Value> result = Nan::RunScript(script.ToLocalChecked());

is returning an undefined but is not empty and the TryCatch isn't catching anything. As far as I can tell it's doing exactly the same thing as it was with NAN@1 (via Handle<Value> result = NanRunScript(script);) but now it's not passing the error back up so I suppose it's to do with other infrastructure in place around it. Ideas?

@kkoopa

This comment has been minimized.

Show comment
Hide comment
@kkoopa

kkoopa Sep 13, 2015

Collaborator

Don't know what's up. It should be hitting https://github.com/nodejs/nan/blob/master/nan.h#L784-L788

NAN_INLINE MaybeLocal<v8::Value> RunScript(
    v8::Local<BoundScript> script
) {
  return MaybeLocal<v8::Value>(script->Run());
}
Collaborator

kkoopa commented Sep 13, 2015

Don't know what's up. It should be hitting https://github.com/nodejs/nan/blob/master/nan.h#L784-L788

NAN_INLINE MaybeLocal<v8::Value> RunScript(
    v8::Local<BoundScript> script
) {
  return MaybeLocal<v8::Value>(script->Run());
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment