n-api: restrict exports by version #19962

kfarnung · 2018-04-12T01:11:06Z

Move napi_get_uv_event_loop into the NAPI_VERSION >= 2 section
Move napi_open_callback_scope, napi_close_callback_scope,
napi_fatal_exception, napi_add_env_cleanup_hook, and
napi_remove_env_cleanup_hook into the NAPI_VERSION >= 3 section
Added a missing added property to napi_get_uv_event_loop in the
docs
Added a napiVersion property to the docs and updated the parser and
generator to use it.
Added usage documentation

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

kfarnung · 2018-04-12T01:15:26Z

An example of what the doc update looks like:

devsnek · 2018-04-12T01:15:29Z

can we just call it napi_version

kfarnung · 2018-04-12T01:34:08Z

I updated it to napiVersion, but I'm open to further feedback.

kfarnung · 2018-04-12T17:45:43Z

Note to self: Add a versioning section to the docs.

mhdawson · 2018-04-12T17:46:49Z

Discussed a bit in the N-API meeting today, Kyle will do a few refinements and then we'll review in detail.

addaleax · 2018-04-21T07:43:48Z

What’s the motivation for restricting exports?

kfarnung · 2018-04-21T21:05:04Z

The idea is that an embedder can declare the version of N-API they want to target and trying to use any newer APIs would result in a build break.

devsnek · 2018-04-21T22:08:47Z

src/node_api.h

@@ -7,6 +7,10 @@

 struct uv_loop_s;  // Forward declaration.

+#ifndef NAPI_VERSION
+  #define NAPI_VERSION  2


with our styles i think this should be

#ifndef NAPI_VERSION #define NAPI_VERSION 2 #endif // NAPI_VERSION

kfarnung · 2018-04-24T00:26:22Z

@mhdawson I bumped the default NAPI_VERSION to 3 since v10.0.0 is coming out soon and we're in the process of backporting the version 3 changes anyway.

mhdawson · 2018-04-24T23:04:04Z

The changes look good but I think we discussed adding a section to the doc which explains how the to use the define etc.

kfarnung · 2018-04-24T23:31:14Z

Yeah, sorry about that, I haven't had a chance to flesh that out yet. Now that 10.0.0 is out I should have a little more time.

BridgeAR · 2018-05-15T15:26:37Z

Ping @kfarnung

kfarnung · 2018-05-15T17:41:22Z

I rebased the changes (pending a build/test locally), I believe I have addressed @mhdawson's comments as well.

kfarnung · 2018-05-15T17:41:42Z

Ping @nodejs/n-api

mhdawson · 2018-05-29T20:58:55Z

@kfarnung I think the one thing we discussed addint to this since you updated was the concept of EXPERIMENTAL that we could use to manage the flow of new functions into N-API.

kfarnung · 2018-05-29T21:13:42Z

That change is on my backlog, I've just been busy with other stuff. I'm hoping to get to that before the N-API meeting on Thursday.

gabrielschulhof

LGTM

kfarnung · 2018-06-28T19:51:25Z

Rebased and squashed: https://ci.nodejs.org/job/node-test-pull-request/15657/

kfarnung · 2018-06-29T00:43:56Z

One more: https://ci.nodejs.org/job/node-test-pull-request/15659/

mhdawson · 2018-06-29T16:12:30Z

src/node_api.h

+
+#ifdef NAPI_EXPERIMENTAL
+
+NAPI_EXTERN napi_status napi_fatal_exception(napi_env env, napi_value err);


I believe fatal_exception is part of NAPI_VERSION 3, it is already exposed in 6.x

mhdawson · 2018-06-29T16:17:18Z

src/node_api.h

+NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env,
+                                                     void (*fun)(void* arg),
+                                                     void* arg);
+


These 2 are already in 10.x but not in 6.x so we probably missed bumping the NAPI_VERSION when they went out.

If we leave as experimental like this PR does we risk breaking somebody, but the same goes if we add a guard for NAPI_VERSION 4. I think we'll need to figure out who added them and figure out what makes the most sense. I do think getting this PR landed would be good though, so maybe just leave them in the regular set (ie non-experimental) for this PR ?

@addaleax What do you think the best approach is? I've moved them to NAPI_VERSION 3 for now.

mhdawson

LGTM once napi_fatal_exception is moved under version 3, and napi_add_env_cleanup_hook and napi_remove_env_cleanup_hook are moved out of experimental

kfarnung · 2018-06-29T22:28:49Z

Rebased: https://ci.nodejs.org/job/node-test-pull-request/15677/

kfarnung · 2018-07-03T16:12:10Z

One more CI: https://ci.nodejs.org/job/node-test-pull-request/15711/

kfarnung · 2018-07-03T16:12:55Z

@mhdawson Any other issues? I'm hoping to land this today, if possible

mhdawson

LGTM

mhdawson · 2018-07-04T19:42:43Z

@kfarnung agreeing we should land, as would like this in place so we can make sure all future API are added as experimental. Can you take the action to follow up on napi_add_env_cleanup_hook/napi_remove_env_cleanup_hook to see if we can move out to NAPI_VERSION 4.x without breaking people?

* Move `napi_get_uv_event_loop` into the `NAPI_VERSION >= 2` section * Move `napi_open_callback_scope`, `napi_close_callback_scope`, `napi_fatal_exception`, `napi_add_env_cleanup_hook`, and `napi_remove_env_cleanup_hook` into the `NAPI_VERSION >= 3` section * Added a missing `added` property to `napi_get_uv_event_loop` in the docs * Added a `napiVersion` property to the docs and updated the parser and generator to use it. * Added usage documentation PR-URL: #19962 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

kfarnung · 2018-07-05T17:42:41Z

Landed in 8476053

* Move `napi_get_uv_event_loop` into the `NAPI_VERSION >= 2` section * Move `napi_open_callback_scope`, `napi_close_callback_scope`, `napi_fatal_exception`, `napi_add_env_cleanup_hook`, and `napi_remove_env_cleanup_hook` into the `NAPI_VERSION >= 3` section * Added a missing `added` property to `napi_get_uv_event_loop` in the docs * Added a `napiVersion` property to the docs and updated the parser and generator to use it. * Added usage documentation PR-URL: #19962 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

* Move `napi_get_uv_event_loop` into the `NAPI_VERSION >= 2` section * Move `napi_open_callback_scope`, `napi_close_callback_scope`, `napi_fatal_exception`, `napi_add_env_cleanup_hook`, and `napi_remove_env_cleanup_hook` into the `NAPI_VERSION >= 3` section * Added a missing `added` property to `napi_get_uv_event_loop` in the docs * Added a `napiVersion` property to the docs and updated the parser and generator to use it. * Added usage documentation PR-URL: nodejs#19962 Backport-PR-URL: nodejs#25648 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

* Move `napi_get_uv_event_loop` into the `NAPI_VERSION >= 2` section * Move `napi_open_callback_scope`, `napi_close_callback_scope`, `napi_fatal_exception`, `napi_add_env_cleanup_hook`, and `napi_remove_env_cleanup_hook` into the `NAPI_VERSION >= 3` section * Added a missing `added` property to `napi_get_uv_event_loop` in the docs * Added a `napiVersion` property to the docs and updated the parser and generator to use it. * Added usage documentation PR-URL: #19962 Backport-PR-URL: #25648 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

kfarnung self-assigned this Apr 12, 2018

kfarnung requested review from addaleax and mhdawson April 12, 2018 01:11

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 12, 2018

kfarnung mentioned this pull request Apr 12, 2018

Enforce NAPI version in module headers nodejs/abi-stable-node#286

Closed

kfarnung added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Apr 12, 2018

devsnek reviewed Apr 21, 2018

View reviewed changes

kfarnung force-pushed the napiver branch 3 times, most recently from 2902039 to 3ce6459 Compare May 4, 2018 01:00

kfarnung force-pushed the napiver branch from 3ce6459 to c0ab388 Compare May 15, 2018 17:37

mhdawson mentioned this pull request May 29, 2018

n-api: add API for asynchronous functions (simpler version) #17887

Closed

4 tasks

gabrielschulhof approved these changes May 31, 2018

View reviewed changes

kfarnung force-pushed the napiver branch from 36380aa to 292a6ed Compare June 28, 2018 19:49

mhdawson reviewed Jun 29, 2018

View reviewed changes

kfarnung force-pushed the napiver branch 2 times, most recently from 86b0958 to fa003a3 Compare June 29, 2018 22:22

mhdawson approved these changes Jul 4, 2018

View reviewed changes

kfarnung closed this Jul 5, 2018

kfarnung force-pushed the napiver branch from fa003a3 to 8476053 Compare July 5, 2018 17:42

kfarnung deleted the napiver branch July 5, 2018 17:42

kfarnung mentioned this pull request Jul 14, 2018

n-api: move cleanup_hook to version 4 #21807

Closed

4 tasks

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

MylesBorins mentioned this pull request Mar 26, 2019

v8.16.0 proposal #26933

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: restrict exports by version #19962

n-api: restrict exports by version #19962

kfarnung commented Apr 12, 2018 •

edited

kfarnung commented Apr 12, 2018

devsnek commented Apr 12, 2018

kfarnung commented Apr 12, 2018

kfarnung commented Apr 12, 2018

mhdawson commented Apr 12, 2018

addaleax commented Apr 21, 2018

kfarnung commented Apr 21, 2018

devsnek Apr 21, 2018 •

edited

kfarnung commented Apr 24, 2018

mhdawson commented Apr 24, 2018

kfarnung commented Apr 24, 2018

BridgeAR commented May 15, 2018

kfarnung commented May 15, 2018

kfarnung commented May 15, 2018

mhdawson commented May 29, 2018

kfarnung commented May 29, 2018

gabrielschulhof left a comment

kfarnung commented Jun 28, 2018

kfarnung commented Jun 29, 2018

mhdawson Jun 29, 2018

mhdawson Jun 29, 2018

kfarnung Jun 29, 2018

mhdawson left a comment

kfarnung commented Jun 29, 2018

kfarnung commented Jul 3, 2018

kfarnung commented Jul 3, 2018

mhdawson left a comment

mhdawson commented Jul 4, 2018

kfarnung commented Jul 5, 2018


		#ifdef NAPI_EXPERIMENTAL

		NAPI_EXTERN napi_status napi_fatal_exception(napi_env env, napi_value err);

n-api: restrict exports by version #19962

n-api: restrict exports by version #19962

Conversation

kfarnung commented Apr 12, 2018 • edited

Checklist

kfarnung commented Apr 12, 2018

devsnek commented Apr 12, 2018

kfarnung commented Apr 12, 2018

kfarnung commented Apr 12, 2018

mhdawson commented Apr 12, 2018

addaleax commented Apr 21, 2018

kfarnung commented Apr 21, 2018

devsnek Apr 21, 2018 • edited

Choose a reason for hiding this comment

kfarnung commented Apr 24, 2018

mhdawson commented Apr 24, 2018

kfarnung commented Apr 24, 2018

BridgeAR commented May 15, 2018

kfarnung commented May 15, 2018

kfarnung commented May 15, 2018

mhdawson commented May 29, 2018

kfarnung commented May 29, 2018

gabrielschulhof left a comment

Choose a reason for hiding this comment

kfarnung commented Jun 28, 2018

kfarnung commented Jun 29, 2018

mhdawson Jun 29, 2018

Choose a reason for hiding this comment

mhdawson Jun 29, 2018

Choose a reason for hiding this comment

kfarnung Jun 29, 2018

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

kfarnung commented Jun 29, 2018

kfarnung commented Jul 3, 2018

kfarnung commented Jul 3, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jul 4, 2018

kfarnung commented Jul 5, 2018

kfarnung commented Apr 12, 2018 •

edited

devsnek Apr 21, 2018 •

edited