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

[v10.x]: Backport instance data #30537

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Nov 18, 2019

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

@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Nov 18, 2019
@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. v10.x labels Nov 18, 2019
@gabrielschulhof
Copy link
Contributor Author

Needed for nodejs/node-addon-api#567

@gabrielschulhof gabrielschulhof added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 18, 2019
@gabrielschulhof gabrielschulhof changed the title v10.x: Backport instance data [v10.x]: Backport instance data Nov 19, 2019
@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

@gabrielf I assume the commit did not apply cleanly. Can you identify the parts you had to update in order to limit what we need to review?

@gabrielschulhof
Copy link
Contributor Author

@mhdawson the separation of js_native_api from node_api is not present in this version, so backporting is not straight-forward. The napi_env in this version is not broken into node_napi_env and napi_env. So, the parts modifying napi_env are the most relevant. Yet the places from which NapiCallIntoModule is being performed had to be updated manually as well, because the files were not separated into node_api and js_native_api, like on master.

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof
Copy link
Contributor Author

Rebased.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof
Copy link
Contributor Author

Rebased again.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof
Copy link
Contributor Author

Rebased again.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax and others added 2 commits December 18, 2019 11:46
These do not need to be macros.

PR-URL: nodejs#26128
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Adds `napi_set_instance_data()` and `napi_get_instance_data()`, which
allow native addons to store their data on and retrieve their data from
`napi_env`. `napi_set_instance_data()` accepts a finalizer which is
called when the `node::Environment()` is destroyed.

This entails rendering the `napi_env` local to each add-on.

Fixes: nodejs/abi-stable-node#378
PR-URL: nodejs#28682
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof
Copy link
Contributor Author

@BethGriggs can we get this into v10.x before it goes into maintenance?

@BethGriggs
Copy link
Member

@gabrielschulhof, yes - we're aiming to have one more semver-minor release in Q1 2020 (probably late Jan/Feb time), I'll land this commit in staging once v10.18.1 has gone out - #30796

@gabrielschulhof
Copy link
Contributor Author

@BethGriggs is this still on track to land?

@BethGriggs
Copy link
Member

BethGriggs commented Feb 19, 2020

@gabrielschulhof yes, but I'm unsure on the timing of the release (nodejs/Release#504 needs updating).

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BethGriggs
Copy link
Member

Possibly will not get a green CI on this until #31887 lands (@AshCripps and I are currently working on backporting the appropriate flaky test markers from v12.x)

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 25, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/29347/ ✅ (Known flake)

BethGriggs pushed a commit that referenced this pull request Feb 25, 2020
These do not need to be macros.

PR-URL: #26128
Backport-PR-URL: #30537
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 25, 2020
Adds `napi_set_instance_data()` and `napi_get_instance_data()`, which
allow native addons to store their data on and retrieve their data from
`napi_env`. `napi_set_instance_data()` accepts a finalizer which is
called when the `node::Environment()` is destroyed.

This entails rendering the `napi_env` local to each add-on.

Fixes: nodejs/abi-stable-node#378
PR-URL: #28682
Backport-PR-URL: #30537
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@BethGriggs
Copy link
Member

Landed in 3f9cec3...f29fb14

@BethGriggs BethGriggs closed this Feb 25, 2020
@gabrielschulhof gabrielschulhof deleted the backport-instance-data branch April 21, 2020 02:58
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++. 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. 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.

6 participants