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

with our styles i think this should be
#ifndefNAPI_VERSION#defineNAPI_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

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

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 <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

* 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 <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

* 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 <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 3ce6459Compare May 4, 2018 01:00

kfarnung force-pushed the napiver branch from 3ce6459 to c0ab388Compare 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 292a6edCompare June 28, 2018 19:49

mhdawson reviewed Jun 29, 2018
View reviewed changes

src/node_api.h Outdated
Copy link
Member
mhdawsonJun 29, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe fatal_exception is part of NAPI_VERSION 3, it is already exposed in 6.x

mhdawson reviewed Jun 29, 2018
View reviewed changes

kfarnung force-pushed the napiver branch 2 times, most recently from 86b0958 to fa003a3Compare 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 8476053Compare 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

Uh oh!

n-api: restrict exports by version#19962

n-api: restrict exports by version #19962

Uh oh!

Conversation

kfarnung commented Apr 12, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

kfarnung commented Apr 12, 2018

Uh oh!

devsnek commented Apr 12, 2018

Uh oh!

kfarnung commented Apr 12, 2018

Uh oh!

kfarnung commented Apr 12, 2018

Uh oh!

mhdawson commented Apr 12, 2018

Uh oh!

addaleax commented Apr 21, 2018

Uh oh!

kfarnung commented Apr 21, 2018

Uh oh!

devsnekApr 21, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfarnung commented Apr 24, 2018

Uh oh!

mhdawson commented Apr 24, 2018

Uh oh!

kfarnung commented Apr 24, 2018

Uh oh!

BridgeAR commented May 15, 2018

Uh oh!

kfarnung commented May 15, 2018

Uh oh!

kfarnung commented May 15, 2018

Uh oh!

mhdawson commented May 29, 2018

Uh oh!

kfarnung commented May 29, 2018

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

kfarnung commented Jun 28, 2018

Uh oh!

kfarnung commented Jun 29, 2018

Uh oh!

mhdawsonJun 29, 2018

Choose a reason for hiding this comment

Uh oh!

mhdawsonJun 29, 2018

Choose a reason for hiding this comment

Uh oh!

kfarnungJun 29, 2018

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

kfarnung commented Jun 29, 2018

Uh oh!

kfarnung commented Jul 3, 2018

Uh oh!

kfarnung commented Jul 3, 2018

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jul 4, 2018

Uh oh!

kfarnung commented Jul 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kfarnung commented Apr 12, 2018•
edited
Loading

devsnekApr 21, 2018•
edited
Loading