chore: bump node to v18.17.0 (main)

[electron-roller]

Updating Node.js to v18.17.0.

See all changes in v18.16.1..v18.17.0

Notes: Updated Node.js to v18.17.0.

[codebytere]

Split things into review blocks to reduce some clutter 😸

WHATWG test failure

Looks like

node feature contexts in renderer URL handling in the renderer process can successfully handle WHATWG URLs constructed by Blink

is failing, with an error i've seen before unfortunately:

"Uncaught TypeError: The "path" argument must be of type string or an instance of Buffer or URL. Received an instance of URL",

Usually this means that Node.js has changed the prototype so it doesn't recognize URLs properly - this is something we'd want to fix since they should both follow WHATWG standards. If i had to guess it's nodejs/node@f495cb6 - isURL probably is returning a false negative in this block via fs.createReadStream. From taking a quick look at nodejs/node#46904 - it looks like we either need to revert what did make it back or pull back more of the changes in that PR

The snapshot Node.js specs failing (parallel/test-snapshot-argv1 & parallel/test-snapshot-namespaced-builtin) we can add to the disabled test roster since we don't support that type of snapshotting at the moment. It also looks like a few of the other failing specs are call stack exceeded errors also rooted in the URL changes 🤔

[ckerr]

Usually this means that Node.js has changed the prototype so it doesn't recognize URLs properly - this is something we'd want to fix since they should both follow WHATWG standards. If i had to guess it's nodejs/node@f495cb6 - isURL probably is returning a false negative in this block via fs.createReadStream. From taking a quick look at nodejs/node#46904 - it looks like we either need to revert what did make it back or pull back more of the changes in that PR

Yes, that's very close to what's happening!

The path to the failure is ReadStream() which calls toPathIfFileUrl() and then validatePath(). The former isn't working right, which is why the latter is throwing the must be of type string or an instance of Buffer or URL exception.

And you're right about that upstream commit: toPathIfFileUrl() is failing because isURL() only returns true for URLs that have a context symbol property, e.g. only URLs created inside of Node. This bug is caused by two upstream factors working together: (1) the main PR you pointed out, nodejs/node#46904, merges isUrl() and isUrlInstance() together. That by itself is OK since it also reimplements isUrl() in an Electron-safe way. The other factor is (2) the backport PR nodejs/node#48345 backports the merging of those two functions but not the new Electron-safe version of isURL().

The easiest answer -- if it works -- would be to also backport the isUrl() implementation from Node's main to 18. I'm going to try that and if the tests come out clean, I'll make an upstream PR.

[ckerr]

Backporting upstream's new implementation as-is causes an infinite loop because it accesses some property getters which, in 18, also call isURL(). 😸 One option would be to backport the URL full refactor, but it looks like upstream hasn't done that because of its size.

1135783 patches around the issue by using both isURL() versions: first it tries the 18 impl and returns true if it succeeds; otherwise, it uses the main impl. This avoids the infinite loop by catching all the Node-generated URLs before using the main impl.

[ckerr]

The Fontconfig error: No writable cache directories error is interesting -- I don't know what's causing it yet, but at least so far it appears to be a real issue that I can repro locally. The fun thing is that Node.js doesn't depend on fontconfig directly, so this is somehow coming from us but apparently didn't make tests go red until now. 🤔

[ckerr]

Quick update on the console test failures: it's coming from fontconfig being unable to find a writable cache dir. Since Electron isn't giving these same warnings, I tried incrementally adding some tracer statements into fontconfig and got these results:

Electron:
[../../third_party/fontconfig/src/src/fcinit.c:80] FcInitLoadOwnConfig
[../../third_party/fontconfig/src/src/fcinit.c:84] FcInitLoadOwnConfig
[../../third_party/fontconfig/src/src/fcinit.c:89] FcInitLoadOwnConfig
[../../third_party/fontconfig/src/src/fcinit.c:91] FcInitLoadOwnConfig
[../../third_party/fontconfig/src/src/fcxml.c:2354] FcParseCacheDir
[../../third_party/fontconfig/src/src/fcxml.c:2367] FcParseCacheDir
[../../third_party/fontconfig/src/src/fcxml.c:2354] FcParseCacheDir
[../../third_party/fontconfig/src/src/fcxml.c:2359] FcParseCacheDir
[../../third_party/fontconfig/src/src/fccfg.c:2515] FcConfigXdgCacheHome
[../../third_party/fontconfig/src/src/fccfg.c:2517] FcConfigXdgCacheHome env '/tmp/charles-cache'
[../../third_party/fontconfig/src/src/fccfg.c:2529] FcConfigXdgCacheHome ret '/tmp/charles-cache'
[../../third_party/fontconfig/src/src/fccfg.c:2546] FcConfigXdgCacheHome ret '/tmp/charles-cache'
[../../third_party/fontconfig/src/src/fcxml.c:2367] FcParseCacheDir
[../../third_party/fontconfig/src/src/fcxml.c:2354] FcParseCacheDir
[../../third_party/fontconfig/src/src/fcxml.c:2367] FcParseCacheDir
[../../third_party/fontconfig/src/src/fcinit.c:104] FcInitLoadOwnConfig
[../../third_party/fontconfig/src/src/fcdir.c:455 FcDirCacheRead
[../../third_party/fontconfig/src/src/fccache.c:1101] FcDirCacheLoad
[../../third_party/fontconfig/src/src/fccache.c:1104] FcDirCacheLoad
[../../third_party/fontconfig/src/src/fccache.c:1107] FcDirCacheLoad
[../../third_party/fontconfig/src/src/fccache.c:367] FcDirCacheProcess
[../../third_party/fontconfig/src/src/fccache.c:373] FcDirCacheProcess
[../../third_party/fontconfig/src/src/fccache.c:382] FcDirCacheProcess
[../../third_party/fontconfig/src/src/fccache.c:389] FcDirCacheProcess
[../../third_party/fontconfig/src/src/fccache.c:403] FcDirCacheProcess '/var/cache/fontconfig//6ee5e371456664662293e51bbe8d7a0d-le64.cache-9'
[../../third_party/fontconfig/src/src/fccache.c:403] FcDirCacheProcess '/tmp/charles-cache/fontconfig//6ee5e371456664662293e51bbe8d7a0d-le64.cache-9'
[../../third_party/fontconfig/src/src/fccache.c:403] FcDirCacheProcess '/home/charles/.fontconfig//6ee5e371456664662293e51bbe8d7a0d-le64.cache-9'
[../../third_party/fontconfig/src/src/fcdir.c:455 FcDirCacheRead

node test:
[../../third_party/fontconfig/src/src/fcinit.c:80] FcInitLoadOwnConfig\n' +
[../../third_party/fontconfig/src/src/fcinit.c:84] FcInitLoadOwnConfig\n' +
[../../third_party/fontconfig/src/src/fcinit.c:89] FcInitLoadOwnConfig\n' +
[../../third_party/fontconfig/src/src/fcinit.c:91] FcInitLoadOwnConfig\n' +
[../../third_party/fontconfig/src/src/fcxml.c:2354] FcParseCacheDir\n' +
[../../third_party/fontconfig/src/src/fcxml.c:2367] FcParseCacheDir\n' +
[../../third_party/fontconfig/src/src/fcxml.c:2354] FcParseCacheDir\n' +
[../../third_party/fontconfig/src/src/fcxml.c:2359] FcParseCacheDir\n' +
[../../third_party/fontconfig/src/src/fccfg.c:2515] FcConfigXdgCacheHome\n' +
[../../third_party/fontconfig/src/src/fccfg.c:2517] FcConfigXdgCacheHome env 'nullptr'\n" +
[../../third_party/fontconfig/src/src/fccfg.c:2546] FcConfigXdgCacheHome ret '/.cache'\n" +
[../../third_party/fontconfig/src/src/fcxml.c:2367] FcParseCacheDir\n' +
[../../third_party/fontconfig/src/src/fcxml.c:2354] FcParseCacheDir\n' +
[../../third_party/fontconfig/src/src/fcxml.c:2367] FcParseCacheDir\n' +
[../../third_party/fontconfig/src/src/fcinit.c:104] FcInitLoadOwnConfig\n' +
[../../third_party/fontconfig/src/src/fcdir.c:455 FcDirCacheRead\n' +
[../../third_party/fontconfig/src/src/fccache.c:1101] FcDirCacheLoad\n' +
[../../third_party/fontconfig/src/src/fccache.c:1104] FcDirCacheLoad\n' +
[../../third_party/fontconfig/src/src/fccache.c:1107] FcDirCacheLoad\n' +
[../../third_party/fontconfig/src/src/fccache.c:367] FcDirCacheProcess\n' +
[../../third_party/fontconfig/src/src/fccache.c:373] FcDirCacheProcess\n' +
[../../third_party/fontconfig/src/src/fccache.c:376] FcDirCacheProcess\n' +
[../../third_party/fontconfig/src/src/fcdir.c:327 FcDirCacheScan\n' +
[../../third_party/fontconfig/src/src/fcdir.c:455 FcDirCacheRead\n' +
[../../third_party/fontconfig/src/src/fccache.c:1101] FcDirCacheLoad\n' +
[../../third_party/fontconfig/src/src/fccache.c:1104] FcDirCacheLoad\n' +
[../../third_party/fontconfig/src/src/fccache.c:1107] FcDirCacheLoad\n' +
[../../third_party/fontconfig/src/src/fccache.c:367] FcDirCacheProcess\n' +
[../../third_party/fontconfig/src/src/fccache.c:373] FcDirCacheProcess\n' +
[../../third_party/fontconfig/src/src/fccache.c:382] FcDirCacheProcess\n' +
[../../third_party/fontconfig/src/src/fccache.c:389] FcDirCacheProcess\n' +
[../../third_party/fontconfig/src/src/fccache.c:403] FcDirCacheProcess '/var/cache/fontconfig//4c599c202bc5c08e2d34565a40eac3b2-le64.cache-9'\n" +
[../../third_party/fontconfig/src/src/fccache.c:403] FcDirCacheProcess '/.cache/fontconfig//4c599c202bc5c08e2d34565a40eac3b2-le64.cache-9'\n" +
[../../third_party/fontconfig/src/src/fcdir.c:327 FcDirCacheScan\n' +

Where my XDG_CACHE_HOME was set to /tmp/charles-cache in both cases.

So apparently XDG_CACHE_HOME is getting unset when running Node.js tests. Tomorrow I'll investigate whether this is coming from inside Node.js, or from the tests, and what to do about it.

[codebytere]

Some comments but nothing blocking :)

[codebytere]

This isn't upstreamable in its current state, but i think it could be! I wouldn't necessarily block on it for this PR, but we can probably address this with something more like this:

Diff Suggestion

diff --git a/test/common/assertSnapshot.js b/test/common/assertSnapshot.js
index 83ee45f5f9..2bc7f0092c 100644
--- a/test/common/assertSnapshot.js
+++ b/test/common/assertSnapshot.js
@@ -62,7 +62,7 @@ async function spawnAndAssert(filename, transform = (x) => x, { tty = false, ...
   const flags = common.parseTestFlags(filename);
   const executable = tty ? 'tools/pseudo-tty.py' : process.execPath;
   const args = tty ? [process.execPath, ...flags, filename] : [...flags, filename];
-  if (options && options.env) options.env.ELECTRON_RUN_AS_NODE = 1;
+  if (options?.env) options.env = { ...process.env, ...options.env };
   const { stdout, stderr } = await common.spawnPromisified(executable, args, options);
   await assertSnapshot(transform(`${stdout}${stderr}`), filename);
 }
diff --git a/test/fixtures/test-runner/output/arbitrary-output-colored.js b/test/fixtures/test-runner/output/arbitrary-output-colored.js
index dd8296e631..0dbaff42d0 100644
--- a/test/fixtures/test-runner/output/arbitrary-output-colored.js
+++ b/test/fixtures/test-runner/output/arbitrary-output-colored.js
@@ -6,6 +6,6 @@ const fixtures = require('../../../common/fixtures');
 
 (async function run() {
   const test = fixtures.path('test-runner/output/arbitrary-output-colored-1.js');
-  await once(spawn(process.execPath, ['--test', test], { stdio: 'inherit', env: { ELECTRON_RUN_AS_NODE: 1, FORCE_COLOR: 1 } }), 'exit');
-  await once(spawn(process.execPath, ['--test', '--test-reporter', 'tap', test], { stdio: 'inherit', env: { ELECTRON_RUN_AS_NODE: 1, FORCE_COLOR: 1 }  }), 'exit');
+  await once(spawn(process.execPath, ['--test', test], { stdio: 'inherit', env: { ...process.env, FORCE_COLOR: 1 } }), 'exit');
+  await once(spawn(process.execPath, ['--test', '--test-reporter', 'tap', test], { stdio: 'inherit', env: { ...process.env, FORCE_COLOR: 1 }  }), 'exit');
 })().then(common.mustCall());
\ No newline at end of file
diff --git a/test/parallel/test-node-output-console.mjs b/test/parallel/test-node-output-console.mjs
index efca7811dc..5a1b9feb6c 100644
--- a/test/parallel/test-node-output-console.mjs
+++ b/test/parallel/test-node-output-console.mjs
@@ -31,7 +31,6 @@ describe('console output', { concurrency: true }, () => {
     .transform(snapshot.replaceWindowsLineEndings, snapshot.replaceWindowsPaths, replaceStackTrace);
   for (const { name, transform, env } of tests) {
     it(name, async () => {
-      if (env) env.ELECTRON_RUN_AS_NODE = 1;
       await snapshot.spawnAndAssert(fixtures.path(name), transform ?? defaultTransform, { env });
     });
   }
diff --git a/test/parallel/test-node-output-errors.mjs b/test/parallel/test-node-output-errors.mjs
index b9a55fb7ea..fca2149fea 100644
--- a/test/parallel/test-node-output-errors.mjs
+++ b/test/parallel/test-node-output-errors.mjs
@@ -53,7 +53,6 @@ describe('errors output', { concurrency: true }, () => {
     !skipForceColors ? { name: 'errors/force_colors.js', env: { FORCE_COLOR: 1 } } : null,
   ].filter(Boolean);
   for (const { name, transform, env } of tests) {
-    if (env) env.ELECTRON_RUN_AS_NODE = 1;
     it(name, async () => {
       await snapshot.spawnAndAssert(fixtures.path(name), transform ?? defaultTransform, { env });
     });

[ckerr]

I like the idea of changing this into some upstreamable form, but unconditionally inheriting all of process.env seems problematic -- I can think of plenty of use cases for a test to intentionally clobber env.

What would you think of having some some mechanism of which vars to inherit, either with a metavariable or passing in some option into tools/test.py?

[codebytere]

Also wouldn't block on it since we'd need to temporarily patch this regardless, but historically we've addressed this by making the snapshot regex matches more generic to account for both Electron and Node.js.

Here's an example of something previously upstreamed: nodejs/node#36489

[ckerr]

Agreed it would be better to find a way to upstream this.

It's using the assertSnapshot mechanism in test/common which is looking for a verbatim snapshot. Looks like the way they handle stacktrace changes upstream is just to periodically set process.env.NODE_REGENERATE_SNAPSHOTS and rebuild the snapshots.

As far as nodejs/node#36489 goes, TBH customizing this test would be easier if it was using that ${testname}.js + ${testname.out} mechanism since that has wildcard support via testcfg.py, e.g. message/assert_throws_stack.out shows how a stacktrace can be wildcarded. But even then, it looks like matching is done on a per-line basis? Since our stacktrace has a different number of lines than upsream, we'd still need another approach.

In order to upstream this I think we'd need to propose some way of doing multiline pattern matching

[release-clerk]

Release Notes Persisted

Updated Node.js to v18.17.0.