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

n-api: restrict exports by version #19962

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
7 participants
@kfarnung
Copy link
Member

commented Apr 12, 2018

  • 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

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2018

An example of what the doc update looks like:

napiver

@devsnek

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

can we just call it napi_version

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2018

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

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2018

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

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

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

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 21, 2018

What’s the motivation for restricting exports?

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2018

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.

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

struct uv_loop_s; // Forward declaration.

#ifndef NAPI_VERSION
#define NAPI_VERSION 2

This comment has been minimized.

Copy link
@devsnek

devsnek Apr 21, 2018

Member

with our styles i think this should be

#ifndef NAPI_VERSION
#define NAPI_VERSION 2
#endif  // NAPI_VERSION
@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2018

@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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2018

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 kfarnung:napiver branch 3 times, most recently from 2902039 to 3ce6459 May 1, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

commented May 15, 2018

Ping @kfarnung

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

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented May 15, 2018

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

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented May 15, 2018

@mhdawson

This comment has been minimized.

Copy link
Member

commented May 29, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented May 29, 2018

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
Copy link
Contributor

left a comment

LGTM

@kfarnung kfarnung force-pushed the kfarnung:napiver branch from 36380aa to 292a6ed Jun 28, 2018

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2018


#ifdef NAPI_EXPERIMENTAL

NAPI_EXTERN napi_status napi_fatal_exception(napi_env env, napi_value err);

This comment has been minimized.

Copy link
@mhdawson

mhdawson Jun 29, 2018

Member

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

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

This comment has been minimized.

Copy link
@mhdawson

mhdawson Jun 29, 2018

Member

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 ?

This comment has been minimized.

Copy link
@kfarnung

kfarnung Jun 29, 2018

Author Member

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

@mhdawson
Copy link
Member

left a 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 kfarnung:napiver branch 2 times, most recently from 86b0958 to fa003a3 Jun 29, 2018

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2018

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2018

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2018

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

@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

@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

n-api: restrict exports by version
* 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 force-pushed the kfarnung:napiver branch from fa003a3 to 8476053 Jul 5, 2018

@kfarnung

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

Landed in 8476053

@kfarnung kfarnung deleted the kfarnung:napiver branch Jul 5, 2018

targos added a commit that referenced this pull request Jul 6, 2018

n-api: restrict exports by version
* 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 referenced this pull request Jul 14, 2018

Closed

n-api: move cleanup_hook to version 4 #21807

4 of 4 tasks complete

@targos targos referenced this pull request Jul 17, 2018

Merged

v10.7.0 proposal #21851

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Jan 25, 2019

n-api: restrict exports by version
* 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>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 4, 2019

n-api: restrict exports by version
* 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 added a commit that referenced this pull request Mar 20, 2019

n-api: restrict exports by version
* 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 referenced this pull request Mar 26, 2019

Merged

v8.16.0 proposal #26933

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.