Skip to content
This repository was archived by the owner on Dec 9, 2018. It is now read-only.

Update for NAN@2 (incomplete) - #181

Closed
rvagg wants to merge 2 commits into
brianmcd:masterfrom
rvagg:nan2
Closed

rvagg wants to merge 2 commits into
brianmcd:masterfrom
rvagg:nan2

Conversation

@rvagg

@rvagg rvagg commented Sep 10, 2015

Copy link
Copy Markdown
Contributor

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

rvagg commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

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

ChALkeR commented Sep 10, 2015

Copy link
Copy Markdown

Adding to the list: nodejs/node#2798.

@mgol

mgol commented Sep 10, 2015

Copy link
Copy Markdown

Related: #180 (comment)

@mgol mgol mentioned this pull request Sep 10, 2015
@brianmcd

Copy link
Copy Markdown
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.

@reggi

reggi commented Sep 12, 2015

Copy link
Copy Markdown

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

Copy link
Copy Markdown
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.

@rvagg

rvagg commented Sep 13, 2015

Copy link
Copy Markdown
Contributor Author

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

kkoopa commented Sep 13, 2015

Copy link
Copy Markdown
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());
}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants