◐ Shell
reader mode source ↗
Skip to content

n-api: restrict exports by version#19962

Closed
kfarnung wants to merge 0 commit into
nodejs:masterfrom
kfarnung:napiver
Closed

n-api: restrict exports by version#19962
kfarnung wants to merge 0 commit into
nodejs:masterfrom
kfarnung:napiver

Conversation

@kfarnung

@kfarnung kfarnung commented Apr 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • 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 kfarnung self-assigned this Apr 12, 2018
@kfarnung kfarnung requested review from addaleax and mhdawson April 12, 2018 01:11
@nodejs-github-bot 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 kfarnung mentioned this pull request Apr 12, 2018
Closed
@kfarnung kfarnung added doc node-api Issues and PRs related to the Node-API. labels Apr 12, 2018
@kfarnung

kfarnung commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

An example of what the doc update looks like:

napiver

@devsnek

devsnek commented Apr 12, 2018

Copy link
Copy Markdown
Member

can we just call it napi_version

@kfarnung

kfarnung commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

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

@kfarnung

kfarnung commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

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

@mhdawson

mhdawson commented Apr 12, 2018

Copy link
Copy Markdown
Member

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

@addaleax

addaleax commented Apr 21, 2018

Copy link
Copy Markdown
Member

What’s the motivation for restricting exports?

@kfarnung

kfarnung commented Apr 21, 2018

Copy link
Copy Markdown
Contributor Author

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.

@kfarnung

kfarnung commented Apr 24, 2018

Copy link
Copy Markdown
Contributor Author

@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

mhdawson commented Apr 24, 2018

Copy link
Copy Markdown
Member

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

kfarnung commented Apr 24, 2018

Copy link
Copy Markdown
Contributor Author

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.

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

BridgeAR commented May 15, 2018

Copy link
Copy Markdown
Member

Ping @kfarnung

@kfarnung

kfarnung commented May 15, 2018

Copy link
Copy Markdown
Contributor Author

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

@kfarnung

kfarnung commented May 15, 2018

Copy link
Copy Markdown
Contributor Author

Ping @nodejs/n-api

@mhdawson

mhdawson commented May 29, 2018

Copy link
Copy Markdown
Member

@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.

@mhdawson mhdawson mentioned this pull request May 29, 2018
Closed
4 tasks
@kfarnung

kfarnung commented May 29, 2018

Copy link
Copy Markdown
Contributor Author

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
gabrielschulhof approved these changes May 31, 2018
View reviewed changes

@gabrielschulhof gabrielschulhof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

60 hidden items Load more…
@kfarnung

kfarnung commented Jun 28, 2018

Copy link
Copy Markdown
Contributor Author

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

@kfarnung

kfarnung commented Jun 29, 2018

Copy link
Copy Markdown
Contributor Author

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

mhdawson
mhdawson reviewed Jun 29, 2018
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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 kfarnung force-pushed the napiver branch 2 times, most recently from 86b0958 to fa003a3 Compare June 29, 2018 22:22
@kfarnung

kfarnung commented Jun 29, 2018

Copy link
Copy Markdown
Contributor Author

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

@kfarnung

kfarnung commented Jul 3, 2018

Copy link
Copy Markdown
Contributor Author

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

@kfarnung

kfarnung commented Jul 3, 2018

Copy link
Copy Markdown
Contributor Author

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

mhdawson
mhdawson approved these changes Jul 4, 2018
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

@mhdawson

mhdawson commented Jul 4, 2018

Copy link
Copy Markdown
Member

@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?

kfarnung added a commit that referenced this pull request Jul 5, 2018
@kfarnung
n-api: restrict exports by version …
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>
@kfarnung kfarnung closed this Jul 5, 2018
@kfarnung

kfarnung commented Jul 5, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 8476053

@kfarnung kfarnung deleted the napiver branch July 5, 2018 17:42
targos pushed a commit that referenced this pull request Jul 6, 2018
@kfarnung @targos
n-api: restrict exports by version …
cec166e 
* 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 kfarnung mentioned this pull request Jul 14, 2018
Closed
4 tasks
@targos targos mentioned this pull request Jul 17, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
Closed
Closed
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 4, 2019
@kfarnung
n-api: restrict exports by version …
80fa044 
* 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>
BethGriggs pushed a commit that referenced this pull request Mar 20, 2019
@kfarnung @BethGriggs
n-api: restrict exports by version …
44609d1 
* 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>
@MylesBorins MylesBorins mentioned this pull request Mar 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@addaleax addaleax Awaiting requested review from addaleax

+2 more reviewers

@devsnek devsnek devsnek left review comments

@mhdawson mhdawson mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kfarnung kfarnung

Labels

c++ doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory. node-api Issues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@kfarnung @devsnek @mhdawson @addaleax @BridgeAR @gabrielschulhof @nodejs-github-bot