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.

Sign up for GitHub

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

Jump to bottom

[v10.x]: Backport instance data #30537

Closed
wants to merge 2 commits into from
Closed

[v10.x]: Backport instance data #30537

wants to merge 2 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Nov 18, 2019 •
edited

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

CI: https://ci.nodejs.org/job/node-test-pull-request/27062/

@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

CI: https://ci.nodejs.org/job/node-test-pull-request/27302/

@gabrielschulhof
Copy link
Contributor Author

Rebased.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27321/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27337/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27342/

@gabrielschulhof
Copy link
Contributor Author

Rebased again.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27368/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27383/

@gabrielschulhof
Copy link
Contributor Author

Rebased again.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27475/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27503/

addaleax and others added 2 commits December 18, 2019 11:46
@addaleax
n-api: turn NAPI_CALL_INTO_MODULE into a function
09776ba 
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>
n-api: add APIs for per-instance state management
dc7d5fd 
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

CI: https://ci.nodejs.org/job/node-test-pull-request/27790/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27797/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27803/

@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 gabrielschulhof mentioned this pull request Jan 8, 2020
Merged
@gabrielschulhof gabrielschulhof mentioned this pull request Jan 19, 2020
Closed
@gabrielschulhof
Copy link
Contributor Author

@BethGriggs is this still on track to land?

@gabrielschulhof gabrielschulhof mentioned this pull request Feb 15, 2020
Closed
4 tasks
@BethGriggs
Copy link
Member

BethGriggs commented Feb 19, 2020 •
edited

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

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29244/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29318/

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

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 25, 2020 •
edited by BethGriggs

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
@addaleax @BethGriggs
n-api: turn NAPI_CALL_INTO_MODULE into a function
20177b9 
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
@BethGriggs
n-api: add APIs for per-instance state management
f29fb14 
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
Reviewers

@gengjiawen gengjiawen gengjiawen approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@gabrielschulhof @nodejs-github-bot @mhdawson @BethGriggs @gengjiawen @addaleax