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

[v8.x-backport] n-api: mark thread-safe function as stable #25648

Conversation

gabrielschulhof
Copy link
Contributor

Remove NAPI_EXPERIMENTAL guard from around N-API thread-safe function
APIs.

Unlike in later versions of Node.js in this version the NAPI_EXPERIMENTAL guard cannot be replaced with #if NAPI_VERSION >= 4 because the N-API versioning commit (8476053) was not backported. So, the only option is to drop the guard.

Fixes: #24249
PR-URL: #25556

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. v8.x labels Jan 22, 2019
@gabrielschulhof gabrielschulhof added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 22, 2019
@gabrielschulhof gabrielschulhof changed the title n-api: mark thread-safe function as stable [v8.x-backport] n-api: mark thread-safe function as stable Jan 25, 2019
@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

@mhdawson I have now backported the commit that introduces the guards.

| v6.x | | | v6.14.2* |
| v8.x | v8.0.0* | v8.10.0* | |
| v9.x | v9.0.0* | v9.3.0* | v9.11.0* |
| v10.x | | | v10.0.0 |
Copy link
Member

Choose a reason for hiding this comment

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

As an aside do we need to update this table, at least in master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

kfarnung and others added 2 commits March 4, 2019 10:29
* 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>
Fixes: nodejs#24249
PR-URL: nodejs#25556
Backport-PR-URL: nodejs#25633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof
Copy link
Contributor Author

Weird ... github said there was a conflict in 4 different files, but I rebased and there was no conflict at all.

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

BethGriggs pushed a commit that referenced this pull request Mar 20, 2019
* 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>
BethGriggs pushed a commit that referenced this pull request Mar 20, 2019
Fixes: #24249
PR-URL: #25556
Backport-PR-URL: #25648
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@BethGriggs
Copy link
Member

Landed on v8.x-staging

@BethGriggs BethGriggs closed this Mar 20, 2019
@gabrielschulhof gabrielschulhof deleted the un-experimental-tsfn-v8.x branch June 14, 2019 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants