FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: expose v8::Isolate setup callbacks by codebytere · Pull Request #35512 · nodejs/node · GitHub

/ node Public

src: expose v8::Isolate setup callbacks - #35512

Closed
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:expose-isolate-callbacks
ConversationCommits (1)ChecksFiles changed
Closed

src: expose v8::Isolate setup callbacks#35512
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:expose-isolate-callbacks

Conversation

codebytere commented Oct 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 5, 2020
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 5, 2020
codebytere force-pushed the expose-isolate-callbacks branch from 587468a to d28751c Compare October 5, 2020 23:36
devsnek approved these changes Oct 5, 2020
View reviewed changes
Comment thread src/env.h Outdated
codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
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 commented Oct 6, 2020

Copy link
Copy Markdown
Collaborator

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

addaleax requested changes Oct 6, 2020
View reviewed changes

addaleax left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 6, 2020

Copy link
Copy Markdown
Member

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 approved these changes Oct 6, 2020
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 6, 2020
codebytere force-pushed the expose-isolate-callbacks branch from d28751c to 48ecb02 Compare October 7, 2020 03:34
codebytere force-pushed the expose-isolate-callbacks branch from 48ecb02 to 09cf132 Compare October 7, 2020 03:35
codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2020
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 commented Oct 7, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Oct 7, 2020

Copy link
Copy Markdown
Collaborator

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 7, 2020

codebytere commented Oct 8, 2020

Copy link
Copy Markdown
Member Author

Landed in ccc822c

codebytere closed this Oct 8, 2020
codebytere deleted the expose-isolate-callbacks branch October 8, 2020 01:33
codebytere added a commit that referenced this pull request Oct 8, 2020
src: expose v8::Isolate setup callbacks
ccc822c 
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
src: expose v8::Isolate setup callbacks
93f947a 
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
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 mentioned this pull request Oct 14, 2020
Merged
MylesBorins added a commit that referenced this pull request Oct 15, 2020
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
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
Closed
Open
Closed
Closed
Closed
Closed
Closed
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 mentioned this pull request Aug 2, 2021
Open
1 task
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
addaleax addaleax approved these changes
juanarbol juanarbol approved these changes
+1 more reviewer
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL