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

Serious issue with NAPI_VERSION being set to 3 on 8.x branch while features are missing #330

Closed
rolftimmermans opened this issue Aug 21, 2018 · 5 comments

Comments

@rolftimmermans
Copy link

I am writing an N-API integration that uses an node::AtExit callback hook, and I intend to use N-API for that.

The documentation of napi_add_env_cleanup_hook https://nodejs.org/api/n-api.html#n_api_napi_add_env_cleanup_hook states this was added in N-API version 3.

However, Node.js 8.11 is missing this function even though it advertises support for N-API version 3.

I do not understand why Node 8 should have N-API version 3, but most of all I'm incredibly surprised by the mismatch between N-API versions and actually exported N-API functions.

I know versions 1-2 were sort of messy, but I definitely expected all N-API version 3 functions to be present if NAPI_VERSION is set to 3.

What is the purpose of NAPI_VERSION if it cannot be used to detect new additions?

Can someone please advise what the best way forward is for module authors like me.

@gabrielschulhof
Copy link
Collaborator

@rolftimmermans I started a backport PR.

@kfarnung
Copy link
Contributor

@rolftimmermans Thanks for the report. Unfortunately NAPI_VERSION 3 was created without a formal process for accepting new APIs and promoting them. That process has been documented now so this should not be an issue going forward.

Thanks @gabrielschulhof for opening the backport PR!

@mhdawson
Copy link
Member

mhdawson commented Aug 22, 2018

Just to add to the comments from @kfarnung, what I understand happened was that a new API was added to master and then backported to 10.X without bumping the NAPI_VERSION which should have been the case. This meant that it would be SemVer major to remove it from 10.X.

The new guidance and use of an experimental phase should prevent this from occurring again.

@gabrielschulhof it will need a backport to both 8.x and 6.x to completely close the gap.

@gabrielschulhof
Copy link
Collaborator

gabrielschulhof commented Aug 22, 2018 via email

@mhdawson
Copy link
Member

Looks like backport PR landed and we have improved the handling of how new functions are added since Node-API went stable. Closing, please let us know if you think that was not the right thing to do.

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

No branches or pull requests

4 participants