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

src: expose v8::Isolate setup callbacks #35512

Closed

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 5, 2020

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

Copy link
Member

@addaleax addaleax left a comment

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 :)

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

@nodejs-github-bot
Copy link
Collaborator

@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
PR-URL: #35512
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 13, 2020
PR-URL: #35512
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 14, 2020
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
MylesBorins added a commit that referenced this pull request Oct 15, 2020
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
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
ryanhc pushed a commit to Samsung/lwnode that referenced this pull request Jun 29, 2022
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
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
Development

Successfully merging this pull request may close these issues.

6 participants