search

nodejs / abi-stable-node

Repository used by the Node-API team to manage work related to Node-API and node-addon-api
239 stars 47 forks source link

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

Closed rolftimmermans closed 2 years ago

rolftimmermans commented 6 years ago

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 commented 6 years ago

@rolftimmermans I started a backport PR.

kfarnung commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@mhdawson good point!

On Wed, Aug 22, 2018 at 5:10 PM, Michael Dawson notifications@github.com wrote:

Just to add to the comments from @kfarnung https://github.com/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 experimental should prevent this from occurring again.

@gabrielschulhof https://github.com/gabrielschulhof it will need a backport to both 8.x and 6.x to completely close the gap.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nodejs/abi-stable-node/issues/330#issuecomment-415181347, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7k0WJ5dhSeWGaon5Ar21ZhuZfyfdjvks5uTcjOgaJpZM4WFfus .

mhdawson commented 2 years ago

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.