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

Fix neon on new node versions, and make it work on Electron as well #123

Closed

Conversation

as-cii
Copy link

@as-cii as-cii commented Oct 25, 2016

This pull request changes neon so that it works on newer node versions. In addition, this pull request will enable using neon from Electron applications as well. ✨ It does so by:

  1. Using the NODE_MODULE_VERSION constant assigned by node-gyp to set the ABI version during register_module!. Using an environment variable should be unnecessary, as node-gyp already takes care of all the work to figure out the modules version for the current node instance.
  2. Fixing v8::ObjectTemplate::Set() with non-primitive values is deprecated #119, which was causing hard crashes on Node >= 6.3.0. In particular, using Function to add methods to a class prototype is now deprecated, and therefore we have changed neon to use FunctionTemplates instead.

@dherman: what do you think? 💭

EdShaw and others added 5 commits September 5, 2016 04:41
If module abi version isn't specified in env, take it from the included node headers.
Signed-off-by: Nathan Sobo <nathan@github.com>
Fixes neon-bindings#119, which was causing hard crashes on newer
versions of node.

Signed-off-by: Nathan Sobo <nathan@github.com>
@EdShaw
Copy link
Contributor

EdShaw commented Nov 2, 2016

IMO, it's probably better to get rid of the version override - of you want to target a specific version, it's going to be safer to acquire the right headers. As such, happy for this to go in instead of my pr.

If there a corresponding change in neon CLI to not set the env var?

@as-cii
Copy link
Author

as-cii commented Nov 2, 2016

IMO, it's probably better to get rid of the version override - of you want to target a specific version, it's going to be safer to acquire the right headers. As such, happy for this to go in instead of my pr.

Agreed, that's why we decided to remove it in this pull request. The idea is that node-gyp will automatically take care of that and there are a lots of ways to force a build against a specific version of node (i.e. environment variables, command line arguments, etc.).

If there a corresponding change in neon CLI to not set the env var?

Yes, we have submitted another pull request (neon-bindings/neon-cli#32) which fixes some Windows problems and takes care of removing the environment variable as well.

@phrohdoh
Copy link

What about this fixes usage with Electron?

@EdShaw
Copy link
Contributor

EdShaw commented Jan 22, 2017

Not sure if latest electron requires the changes for node compatibility.

The node Abi changes "fixes" electron by removing the assumption that your building for the node that you're running neon-cli with.

@dherman
Copy link
Collaborator

dherman commented Mar 11, 2017

OK, this PR gave us lots and lots of helpful info and we're making good progress. Windows support is in pretty good shape now, and #194 is tracking the remaining steps we need for Electron support. So I'm closing this one with much gratitude!

@dherman dherman closed this Mar 11, 2017
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.

4 participants