The Wayback Machine - https://web.archive.org/web/20220423193250/https://github.com/nodejs/node/pull/35512
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

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 label Oct 5, 2020
@nodejs-github-bot nodejs-github-bot added the c++ label Oct 5, 2020
src/env.h Outdated Show resolved Hide resolved
@codebytere codebytere added the request-ci label Oct 6, 2020
@github-actions github-actions bot removed the request-ci label Oct 6, 2020
Copy link
Member

@addaleax addaleax left a comment

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 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
Copy link
Member

@Trott Trott left a comment

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 label Oct 6, 2020
@codebytere codebytere added the request-ci label Oct 7, 2020
@github-actions github-actions bot removed the request-ci label Oct 7, 2020
@addaleax addaleax added the author ready label Oct 7, 2020
@codebytere
Copy link
Member Author

@codebytere codebytere commented Oct 8, 2020

Landed in ccc822c

@codebytere codebytere closed this Oct 8, 2020
@codebytere codebytere deleted the expose-isolate-callbacks branch Oct 8, 2020
codebytere added a commit that referenced this issue Oct 8, 2020
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 issue Oct 13, 2020
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 issue 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 issue 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 issue 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
@renovate renovate bot mentioned this pull request Aug 2, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready c++ embedding semver-minor
6 participants