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

src: expose v8::Isolate setup callbacks #35512

Closed
wants to merge 1 commit into from
Closed

src: expose v8::Isolate setup callbacks #35512

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 5, 2020 •
edited
Loading

This PR makes some more of the v8::Isolate* setup callbacks used by Node.js to external embedders. We already possess the ability to override the callbacks used, but there are some situations in which we would still want to access Node.js' default behavior.

For example: in an Electron renderer process, the context currently in play would not have been subject to some of the ContextEmbedderData data set by Node.js, but the v8::Isolate would assume that it had and make erroneous choices as a result. As such, in this situation, we would tell the isolate to make different choices in the callback depending on the current process. We could, of course, simply copy over the code, but we then run the risk of missing future changes to this callback logic. There is also precedent for this - we can already access task_queue::PromiseRejectCallback for similar ends.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 5, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 5, 2020
@codebytere codebytere force-pushed the expose-isolate-callbacks branch from 587468a to d28751c Compare October 5, 2020 23:36
devsnek
devsnek approved these changes Oct 5, 2020
View reviewed changes
src/env.h Outdated Show resolved Hide resolved
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
@nodejs-github-bot
Copy link
Collaborator

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

addaleax
addaleax requested changes Oct 6, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I’m definitely 👍 on this, but as supported public APIs they should go into node.h instead of the internal Environment class (and use NODE_EXTERN)

@addaleax
Copy link
Member

addaleax commented Oct 6, 2020

There is also precedent for this - we can already access task_queue::PromiseRejectCallback for similar ends.

Can you move that to node.h as well? Electron can access it, but embedders who stick to supported APIs can’t yet :)

Trott
Trott approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Concept SGTM. And implementation per @addaleax's suggestion SGTM. Will want to add a semver-minor label in that case too?

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 6, 2020
@codebytere codebytere force-pushed the expose-isolate-callbacks branch from d28751c to 48ecb02 Compare October 7, 2020 03:34
@codebytere codebytere force-pushed the expose-isolate-callbacks branch from 48ecb02 to 09cf132 Compare October 7, 2020 03:35
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 7, 2020
@codebytere
Copy link
Member Author

Landed in ccc822c

@codebytere codebytere closed this Oct 8, 2020
@codebytere codebytere deleted the expose-isolate-callbacks branch October 8, 2020 01:33
codebytere added a commit that referenced this pull request Oct 8, 2020
@codebytere
src: expose v8::Isolate setup callbacks
ccc822c 
PR-URL: #35512
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 13, 2020
@codebytere @BethGriggs
src: expose v8::Isolate setup callbacks
93f947a 
PR-URL: #35512
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins added a commit that referenced this pull request Oct 14, 2020
@MylesBorins
2020-10-15, Version 14.14.0 (Current)
6112962 
Notable changes:

crypto:
  * update certdata to NSS 3.56 (Shelley Vohr) #35546
doc:
  * add aduh95 to collaborators (Antoine du Hamel) #35542
fs:
  * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494
http:
  * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274
src:
  * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512

PR-URL: TODO
@MylesBorins MylesBorins mentioned this pull request Oct 14, 2020
Merged
MylesBorins added a commit that referenced this pull request Oct 15, 2020
@MylesBorins
2020-10-15, Version 14.14.0 (Current)
0ea4bd2 
Notable changes:

crypto:
  * update certdata to NSS 3.56 (Shelley Vohr) #35546
doc:
  * add aduh95 to collaborators (Antoine du Hamel) #35542
fs:
  * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494
http:
  * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274
src:
  * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512

PR-URL: TODO
MylesBorins added a commit that referenced this pull request Oct 15, 2020
@MylesBorins
2020-10-15, Version 14.14.0 (Current)
354b6a9 
Notable changes:

crypto:
  * update certdata to NSS 3.56 (Shelley Vohr) #35546
doc:
  * add aduh95 to collaborators (Antoine du Hamel) #35542
fs:
  * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494
http:
  * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274
src:
  * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512

PR-URL: #35648
This was referenced Jun 23, 2021
Open
Closed
Closed
Closed
Closed
Closed
Merged
Merged
This was referenced Jul 1, 2021
Merged
Merged
Closed
Closed
Merged
This was referenced Jul 11, 2021
Closed
Open
This was referenced Jul 23, 2021
Merged
Merged
Merged
Open
Merged
This was referenced Jul 31, 2021
Merged
Merged
Closed
Closed
Closed
@gary-kim-bot gary-kim-bot mentioned this pull request Aug 2, 2021
Open
1 task
@renovate renovate bot mentioned this pull request Aug 3, 2021
Merged
1 task
This was referenced Aug 11, 2021
Closed
Open
ryanhc pushed a commit to Samsung/lwnode that referenced this pull request Jun 29, 2022
@MylesBorins
2020-10-15, Version 14.14.0 (Current)
edc407f 
Notable changes:

crypto:
  * update certdata to NSS 3.56 (Shelley Vohr) nodejs/node#35546
doc:
  * add aduh95 to collaborators (Antoine du Hamel) nodejs/node#35542
fs:
  * (SEMVER-MINOR) add rm method (Ian Sutherland) nodejs/node#35494
http:
  * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) nodejs/node#35274
src:
  * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) nodejs/node#35512

PR-URL: nodejs/node#35648
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanarbol juanarbol juanarbol approved these changes

@devsnek devsnek devsnek approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. 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.
6 participants
@codebytere @nodejs-github-bot @addaleax @Trott @devsnek @juanarbol