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

remove N-API implementation and v8.x support #643

Closed

Conversation

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jan 1, 2020

Remove the files associated with the external implementation of N-API
and the v8.x Travis CI testing. This move is possible because of v8.x
EOL, which means that all supported versions of Node.js now have an
internal implementation of N-API.

Fixes: #463
Fixes: #509

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:remove-implementation branch from fea8e93 to 2873f39 Jan 1, 2020
@gabrielschulhof gabrielschulhof changed the title remove N-API implementation remove N-API implementation and v8.x support Jan 1, 2020
@legendecas

This comment has been minimized.

Copy link
Member

legendecas commented Jan 2, 2020

This can fix #640 since it removes Node.js v6 support.

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:remove-implementation branch from 2873f39 to 2acbc3c Jan 2, 2020
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jan 2, 2020

@legendecas thanks! I updated the commit message to reflect that.

Copy link
Member

NickNaso left a comment

What do you think to move node_api.gyp and nothing.c on the root folder and completely remove the src folder? Maybe now we could change the name of node_api.gyp in nothing.gyp and then use nothing.gyp:nothing on index.js.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jan 3, 2020

@NickNaso we should also update the docs to remove 'dependencies': [ '<!(...)'].

@NickNaso

This comment has been minimized.

Copy link
Member

NickNaso commented Jan 3, 2020

@NickNaso we should also update the docs to remove 'dependencies': [ '<!(...)'].

Yes, in this way the new end-user will not include the dependencies in their binding.gyp, but the old project will continue to work because under the hood we will continue to provide / export the dependencies.

Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: #463
Fixes: #509
Fixes: #640
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:remove-implementation branch from 2acbc3c to 88d0032 Jan 3, 2020
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jan 3, 2020

@tniessen @legendecas @NickNaso I updated the PR to include a doc update removing instructions for adding the external N-API implementation to an addon's dependencies. Could you please take another look?

Copy link
Member

NickNaso left a comment

LGTM

Copy link
Member

mhdawson left a comment

LGTM

gabrielschulhof added a commit that referenced this pull request Jan 7, 2020
Remove the followings:

* the files associated with the external implementation of N-API
* Travis CI jobs for v8.x and v6.x
* documentation instructing users to add the external N-API
  implementation to their dependencies.
* conversion tool code that adds the external N-API implementation as a
  dependency to the user's addon.

This move is possible because of v8.x EOL, which means that all
supported versions of Node.js now have an internal implementation of
N-API.

Fixes: #463
Fixes: #509
Fixes: #640
PR-URL: #643
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jan 7, 2020

Landed in 9af69da.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 10, 2020

@gabrielschulhof Just to make sure, this will be released in a new semver-major release, right?

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jan 10, 2020

@addaleax good point! It definitely should be semver-major.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.