-
Notifications
You must be signed in to change notification settings - Fork 48
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
Compatibility with Node 10.+ #54
Comments
What errors did you see? Can you provide more details? We currently provide pre-built packages on npm for Node 4, 6, 8. If you want to add support for Node 10 you can modify the |
Will do. Thanks 👍 |
Hey, @daniel-j-h I'm not sure if this connected with node 10 (in my case 10.3.0) or not, but can't install the module, am I doing something wrong?
Don't I have any -dev package system-wide? |
Seems like this is a breaking change in the v8 API
coming from
You can simply remove the branches for now and re-compile. |
@daniel-j-h sorry Daniel, what branches are you talking about? |
In tsp.cc and vrp.cc remove Lines 28 to 33 in 30351e1
and Lines 27 to 32 in 30351e1
The |
Is this fix going to be release officially? |
Happy to review a pull request if you want to help out here. It's not as easy as removing the two branches above as it will break compatibility with user code. But otherwise there should be nothing blocking this. |
I have submitted a PR that I believe should fix the issue and preserve compatibility. |
Thanks! I merged your pull request, released v1.0.6. Travis automatically built binaries for LTS Node 4,6,8. |
Had to downgrade to Node.js 9.7.1 to make it work...
The text was updated successfully, but these errors were encountered: