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

chore: bump Node.js to v16.2.0 #29244

Merged
merged 69 commits into from Jun 17, 2021
Merged

chore: bump Node.js to v16.2.0 #29244

merged 69 commits into from Jun 17, 2021

Conversation

codebytere
Copy link
Member

Description of Change

Upgrading Node.js to v16.2.0. A work in progress, which ideally will go out in Electron v14.

Checklist

Release Notes

Notes: : Updated Node.js to v16.2.0.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 19, 2021
@codebytere codebytere force-pushed the upgrade-node-16 branch 3 times, most recently from fe422d2 to c827e97 Compare May 20, 2021 13:11
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 26, 2021
@codebytere codebytere added the semver/major incompatible API changes label May 31, 2021
@codebytere codebytere force-pushed the upgrade-node-16 branch 7 times, most recently from 61b4278 to 590bcf4 Compare June 3, 2021 17:36
patches/node/build_add_gn_build_files.patch Show resolved Hide resolved
script/node-disabled-tests.json Outdated Show resolved Hide resolved
script/node-disabled-tests.json Outdated Show resolved Hide resolved
script/node-disabled-tests.json Outdated Show resolved Hide resolved
spec-main/api-browser-window-spec.ts Outdated Show resolved Hide resolved
spec-main/extensions-spec.ts Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the upgrade-node-16 branch 6 times, most recently from f9daf4b to fdaacbd Compare June 9, 2021 08:59
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. This is looking good. +1 on Jeremy's comments about crypto.

Also I lean towards Anna's idea upstream about finding a way to handle unhandled exceptions without adding new undocumented API. Also, I I'm still curious why it's needed, so one nit on this PR might be to update the patch comment to add more background explaining that 🙂


This PR adds a get/set pair for unhandled rejections modes.

We do not want unhandled rejections to crash the process and want to
Copy link
Member

Choose a reason for hiding this comment

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

@codebytere I'm curious about this too. What's prompting this patch?

@codebytere codebytere marked this pull request as ready for review June 14, 2021 14:32
@codebytere codebytere requested a review from a team as a code owner June 14, 2021 14:32
@codebytere
Copy link
Member Author

codebytere commented Jun 14, 2021

@ckerr @nornagon in v15 Node.js made a change such that unhandled rejections exit/crash the process. I introduced this patch because I think this behavior isn't appropriate for all Electron processes, and that we instead want to selectively enable it. For example, I think that this behavior is fine/expected in ELECTRON_RUN_AS_NODE, but that an unhandled rejection shouldn't crash the renderer process.

@MarshallOfSound
Copy link
Member

Also I lean towards Anna's idea upstream about finding a way to handle unhandled exceptions without adding new undocumented API

To clarify this, the "undocumented API" is internal to NodeJS in cpp land, there's no public facing API here from Electrons perspective.

Do we not? Why do we want to differ from node in this regard?

To answer this question though, we already differ from Node in regards to global error handling for precisely the reasons @codebytere outlined above. What is expected behavior for a NodeJS script / server is entirely unexpected for a desktop application. One unhandled rejection / exception should not pull the entire app down.

That's actually why we have this global uncaughtException handler which prevents nodes default behavior of bringing down the process because by default in electron that error is handled.

The approach we took here is slightly different (rather we made the unhandled rejection behavior it's old-form which people expect) but an equivalent solution would be to add a similar global unhandledRejection listener such that all rejections are "handled" still.

The TLDR here is expectations of unhandled errors are fundamentally different between desktop apps and node scripts.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

lgtm overall, v happy to see all the upstreamed crypto stuff!

@release-clerk
Copy link

release-clerk bot commented Jun 17, 2021

Release Notes Persisted

: Updated Node.js to v16.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants