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

Enforce NAPI version in module headers #286

Closed
kfarnung opened this issue Dec 7, 2017 · 7 comments
Closed

Enforce NAPI version in module headers #286

kfarnung opened this issue Dec 7, 2017 · 7 comments
Milestone

Comments

@kfarnung
Copy link
Contributor

kfarnung commented Dec 7, 2017

As discussed today it seems like rather than having developers declare their version specifically it would make sense to enforce it in the header file.

By default only the first version would be exposed and the developer can explicitly opt in using a preprocessor directive:

#define NAPI_VERSION  2
#include <node_api.h>

I've made the required changes in a private branch to start the discussion. If there are no objections I can go ahead and open a PR in the main repo.

@mhdawson
Copy link
Member

Looks reasonable. I think we'll need a doc that captures the approach and update the doc on creating a release so that we include the required steps.

I also think we should discuss if we start enforcing this before/after we exit experimental.

@kfarnung
Copy link
Contributor Author

@mhdawson I can update the documentation for N-API to describe the approach for module authors, is that sufficient?

It would probably make sense to have an answer for this before we exit experimental, but there are more pieces to put into place. I'm not sure how this would fit into the end-to-end pipeline, for instance how do modules surface this value to npm/node-gyp?

@mhdawson
Copy link
Member

Updating the N-API documentation works for me.

@mhdawson mhdawson added this to the Milestone 10 milestone Mar 15, 2018
@kfarnung
Copy link
Contributor Author

kfarnung commented Apr 5, 2018

Based on discussion in the meeting today we'll set the starting version at 2 (the current shipping N-API version) so we don't break any developers.

@kfarnung
Copy link
Contributor Author

I'm still working on this PR, but I took a stab at trying to figure out the version mapping:

1 2 3
v4.x
v6.x
v8.x v8.0.0* v8.10.0*
v9.x v9.0.0* v9.3.0*
v10.x v10.0.0

* Versions 1 and 2 were really loose (during experimental) so these are approximate.

@kfarnung
Copy link
Contributor Author

I opened the initial PR: nodejs/node#19962

@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2018

PR landed, closing out.

@mhdawson mhdawson closed this as completed Jul 5, 2018
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

2 participants