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: allow preventing SetPromiseRejectCallback #34387

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jul 16, 2020

This PR slightly augments the existing IsolateSettings struct for embedder purposes.

Electron does not want to use the promise rejection callback that Node.js uses,
because it does not send PromiseRejectionEvents to the global script context in the browser (for fairly obvious reasons).

Therefore, if we want to use Node.js setup logic in the renderer process (as opposed to standalone embedded mode, where we already do this) we would not be able to allow users to do things such as:

window.addEventListener('unhandledrejection', function (event) {
  // ...your code here to handle the unhandled rejection...

  // Prevent the default handling (such as outputting the
  // error to the console)

  event.preventDefault();
});

since the 'unhandledrejection' event would never be fired.

We need to use the one Blink already provides and sets on the Isolate we're given, and so we need to slightly augment IsolateSettings to allow for that.

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

@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Jul 16, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 16, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 16, 2020
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.

LGTM, but I think it makes sense to use the inverse flag (i.e. SHOULD_NOT_SET_PROMISE_REJECTION_CALLBACK), because then this becomes semver-minor instead :)

@codebytere codebytere force-pushed the allow-preventing-SetPromiseRejectCallback branch from 0e4958e to 8862256 Compare July 16, 2020 17:43
@codebytere codebytere added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 16, 2020
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 17, 2020
@nodejs-github-bot
Copy link
Collaborator

src/node.h Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the allow-preventing-SetPromiseRejectCallback branch from 5eec3ac to 267c4c9 Compare July 20, 2020 16:11
src/api/environment.cc Outdated Show resolved Hide resolved
@nodejs nodejs deleted a comment from nodejs-github-bot Jul 20, 2020
@nodejs nodejs deleted a comment from nodejs-github-bot Jul 20, 2020
@codebytere codebytere force-pushed the allow-preventing-SetPromiseRejectCallback branch from 267c4c9 to a7a5972 Compare July 20, 2020 17:03
@nodejs-github-bot
Copy link
Collaborator

MylesBorins pushed a commit that referenced this pull request Jul 27, 2020
PR-URL: #34387
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
ruyadorno added a commit that referenced this pull request Jul 28, 2020
Notable changes:

dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057

PR-URL: TODO
@ruyadorno ruyadorno mentioned this pull request Jul 28, 2020
ruyadorno added a commit that referenced this pull request Jul 28, 2020
Notable changes:

dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
ruyadorno added a commit that referenced this pull request Jul 29, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.7 (claudiahdz) #34468
dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
MylesBorins pushed a commit that referenced this pull request Jul 29, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.7 (claudiahdz) #34468
dgram:
  * (SEMVER-MINOR) add IPv6 scope id suffix to received udp6 dgrams (Pekka Nikander) #14500
doc:
  * add AshCripps to collaborators (AshCripps) #34494
  * add HarshithaKP to collaborators (Harshitha K P) #34417
  * add rexagod to collaborators (Pranshu Srivastava) #34457
  * add release key for Richard Lau (Richard Lau) #34397
events:
  * (SEMVER-MINOR) expand NodeEventTarget functionality (Anna Henningsen) #34057
src:
  * (SEMVER-MINOR) allow preventing SetPromiseRejectCallback (Shelley Vohr) #34387
  * (SEMVER-MINOR) allow setting a dir for all diagnostic output (AshCripps) #33584
worker:
  * (SEMVER-MINOR) make MessagePort inherit from EventTarget (Anna Henningsen) #34057
zlib:
  * switch to lazy init for zlib streams (Andrey Pechkurov) #34048

PR-URL: #34542
codebytere added a commit to electron/electron that referenced this pull request Sep 1, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 1, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 2, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 4, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 16, 2020
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34387
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@codebytere codebytere mentioned this pull request Sep 28, 2020
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.

None yet

6 participants