chore: upgrade to Node.js v18 #35999
Conversation
3621500
to
fe16af8
Compare
f85e532
to
a9fdc59
Compare
88c4b58
to
023e065
Compare
62c1a30
to
9a98ccb
Compare
| rename from deps/base64/base64/lib/arch/avx/codec.c | ||
| rename to deps/base64/base64/lib/arch/avx/avx_codec.c |
There was a problem hiding this comment.
huh, how come we needed to rename these?
There was a problem hiding this comment.
gn doesn't seem to allow duplicate filenames even if they're in different directories - this is what happens otherwise:
electron on git:upgrade-nodejs-v18 ❯ e build 10:12AM
Running "e load-xcode --quiet"
Running "python3 goma_auth.py info" in /Users/codebytere/.electron_build_tools/third_party/goma
Running "ninja -j 200 electron" in /Users/codebytere/Developer/electron-gn/src/out/Testing
[0/1] Regenerating ninja files
ERROR at //third_party/electron_node/deps/base64/BUILD.gn:10:1: Duplicate object file
static_library("base64") {
^-------------------------
The target //third_party/electron_node/deps/base64:base64
generates two object files with the same name:
obj/third_party/electron_node/deps/base64/base64/codec.o
It could be you accidentally have a file listed twice in the
sources. Or, depending on how your toolchain maps sources to
object files, two source files with the same name in different
directories could map to the same object file.
In the latter case, either rename one of the files or move one of
the sources to a separate source_set to avoid them both being in
the same target.
FAILED: build.ninja.stamp
There was a problem hiding this comment.
Generally the way to solve this in GN is to use separate targets. Make an base64_avx target or something that has the avx-specific files in it. object names only need to be unique within a single target.
| @@ -117,6 +117,7 @@ | |||
| "parallel/test-trace-events-v8", | |||
| "parallel/test-trace-events-vm", | |||
| "parallel/test-trace-events-worker-metadata", | |||
| "parallel/test-wasm-web-api", | |||
There was a problem hiding this comment.
@nornagon i can add a follow-up for this but what's breaking would require a refactor far (imo) beyond the scope of this PR. Essentially, WebAssembly.compileStreaming and WebAssembly.instantiateStreaming are undefined when they shouldn't be. That's happening because Node.js' logic for enabling them here is called via node::SetIsolateUpForNode, which by necessity can only be called on an existing isolate. However, the switch to enable it here is called by gin during its own isolate initialization, which means that it ends up being undefined. I can look into a good way to fix this but imo impact is low and refactor burden is high so i'd prefer to just disable it for now.
There was a problem hiding this comment.
Fair enough, happy to leave this for a followup, but it should probably be a release blocker as those methods are expected to be available in this version of Node.
bootstrap: implement run-time user-land snapshots via --build-snapshot and --snapshot-blob
|
Release Notes Persisted
|
| import { net, session, ClientRequest, BrowserWindow, ClientRequestConstructorOptions } from 'electron/main'; | ||
| import * as http from 'http'; | ||
| import * as url from 'url'; | ||
| import { AddressInfo, Socket } from 'net'; | ||
| import { emittedOnce } from './events-helpers'; | ||
| import { defer, delay } from './spec-helpers'; | ||
|
|
||
| // See https://github.com/nodejs/node/issues/40702. | ||
| dns.setDefaultResultOrder('ipv4first'); |
There was a problem hiding this comment.
This "fix" should be reverted. It causes problems for lots of people using IPv6.
There was a problem hiding this comment.
Better use net.autoSelectFamily instead which enabled Happy Eyeballs according to RFC 8305 (requires upgrading to node v18.13).
There was a problem hiding this comment.
Also why is this change not addressed in the release notes in this PR?
This change is not in the upstream and some downstream projects where expecting this option to change as per v17 release notes.
If there is a problem with it, it should raised as an issue of this project, discussed with the community and fixed accordingly. Not by silently hiding it in large change.
Description of Change
Upgrade to Node.js v18.
Checklist
npm testpassesRelease Notes
Notes: Upgraded Node.js to v18.10.0