Skip to content
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

Nan API may have changed in Node 12 #41

Closed
rubensworks opened this issue May 3, 2019 · 4 comments · Fixed by #43
Closed

Nan API may have changed in Node 12 #41

rubensworks opened this issue May 3, 2019 · 4 comments · Fixed by #43

Comments

@rubensworks
Copy link
Member

rubensworks commented May 3, 2019

Recently, I started having Travis failures when using this lib on Node 12.
Starting from this line: https://travis-ci.org/comunica/comunica-actor-rdf-resolve-quad-pattern-hdt/jobs/527504675#L661

The same problem occurs in OSTRICH, which I still have to fix:
rdfostrich/ostrich-node#3

Failing builds:

@rubensworks rubensworks changed the title Nan API may have changed Nan API may have changed in Node 11 May 3, 2019
@rubensworks rubensworks changed the title Nan API may have changed in Node 11 Nan API may have changed in Node 12 May 3, 2019
@rubensworks
Copy link
Member Author

After looking into it a bit, this seems to be a problem with Nan, as they still have to update to Node 12.
See nodejs/nan#849

@Flarna
Copy link

Flarna commented May 3, 2019

I took a fast look into the error messages and there is more to do then update NAN.
e.g. at https://github.com/RubenVerborgh/HDT-Node/blob/master/lib/HdtDocument.cc#L69 there is a direct use of a v8 API which is no longer existing in NodeJs 12 (v8::Local<v8::FunctionTemplate>::GetFunction()).

NAN provides already an API for this in 2.13.2, e.g. use Nan::GetFunction(constructorTemplate).ToLocalChecked()

@mielvds
Copy link
Collaborator

mielvds commented Dec 3, 2019

#42 can indeed be closed. Can somebody PR this? I'm afraid I don't know NAN or c++ well enough to give it a go.

@mielvds
Copy link
Collaborator

mielvds commented Dec 3, 2019

Decided to take a swing at it in #43 , effort was ok in the end.

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 a pull request may close this issue.

3 participants