Update the minimum supported build-time version of node to 16.20.0 #20551

sbc100 · 2023-10-27T01:56:57Z

Bump the minimum build-time version of node 10.19.0 -> 16.20.0

This new minimum version matches the version that we ship with emsdk.
Critically it supports null coalescing & logical assignment needed
by #20549.

curiousdannii · 2023-10-27T04:47:43Z

Node 16 is already EOL (by one month). Would it be okay to push the MSV to 18?

RReverser

I don't know what the implications of bumping Node version are, but LGTM.

RReverser · 2023-10-27T11:26:03Z

Node 16 is already EOL (by one month). Would it be okay to push the MSV to 18?

I suspect this would need to change in emsdk first.

RReverser · 2023-10-27T14:02:12Z

Nit: can't comment on specific line as it's not part of the diff, but node_pthread_flags() also need to be removed.

RReverser · 2023-10-27T14:07:08Z

Also the code/comment here

emscripten/emcc.py

Line 2279 in 1dedd1b

# 10.19.0 is the oldest version of node that we do any testing with.

sbc100 · 2023-10-27T15:52:43Z

Node 16 is already EOL (by one month). Would it be okay to push the MSV to 18?

The MSV of node for running emscripten tends to lag way behind the officially supported node versions (as you can see from the fact that we are still supporting node 10.19.0!) since LTS linux distros such as debian/stable and ubuntu LTS.

dschuff · 2023-10-27T17:03:28Z

Is 18 the version where the official binaries don't run on certain older Linux distros?

RReverser · 2023-10-27T17:03:50Z

Nit: can't comment on specific line as it's not part of the diff, but node_pthread_flags() also need to be removed.

@sbc100 I think this comment still stands.

sbc100 · 2023-10-27T17:07:00Z

Is 18 the version where the official binaries don't run on certain older Linux distros?

Yes. Specifically they dropped support for ubuntu/trusty.

emcc.py

src/settings.js

ChangeLog.md

kripken · 2023-10-27T17:26:12Z

emcc.py

-    if settings.MIN_NODE_VERSION >= 150000:
-      default_setting('NODEJS_CATCH_REJECTION', 0)
+    if settings.MIN_NODE_VERSION < 162000:
+      exit_with_error('targeting node older than v16.2 is not supported')


Wait, I'm confused - MIN_NODE_VERSION is for the generated code, which contradicts the changelog entry.

Are both being updated in this PR?

If we want to change the generated code version, can we not support older node by using transpilation?

Sorry you are correct, I also bumped min runtime version we support from 150000 to 162000. This keeps the testing matrix a little simple since it avoids us having to do specific testing with v15. But maybe its worth keeping this change focused on the build-time version.

I decided to stick with the update of both version. The min output version is much more minor bump

kripken · 2023-10-27T18:19:16Z

ChangeLog.md

 -----------------------
+- The minimum version of node required run the compiler was updated from
+  10.19 to 16.20.  The minimum verison of node supported by the
+  emscripten-generated code was updated from 15.0 to 16.20.  One side effect of


The code below this in emcc.py shows this bumps the absolute version (the version we errored on, and do not even support transpiling on) for generated code from 10 to 16. So this comment looks flipped?

A hard error on node 16.20 is fairly strict. Is it hard to support earlier versions using transpilation, as I asked before? If it is, I'm not necessarily opposed to this, but let's then point people to using older versions of emscripten. The reason I mention all this is that sometimes someone does want to run "everywhere", even old machines without outdated node.

I think transpilation + users supplying their own Node flags should still work if we remove the hard error, it's just that we won't test those versions or provide helpful messages (we don't want to maintain those forever).

OK, perhaps I'll just downgrade this to a warning. I'm trying to avoid testing on yet another version of node.

Yeah I think that will be sensible middle ground.

The default is already higher than one specified here, so it wasn't really a bump. This is a tiny part extracted from emscripten-core#20551 just to unblock this build.

This is needed for emscripten-core#20551 so that we can continue to run tests under versions of node that are too old to run the compiler.

This is needed for #20551 so that we can continue to run tests under versions of node that are too old to run the compiler.

sbc100 · 2023-10-30T23:36:53Z

Now that #20573 has landed this change is once again solely focused on the build time version of node. PTAL

This new minimum version matches the version that we ship with emsdk. Critically it supports null coalescing & logical assignment needed by emscripten-core#20549.

kripken · 2023-10-30T23:57:55Z

ChangeLog.md

 -----------------------
+- The minimum version of node required run the compiler was updated from
+  10.19 to 16.20.  This does not effect the node requirements of the generated
+  JavaScript code. (#20551)


Perhaps rename the PR to be like this comment? "build-time" is less clear than the text here, "to run the compiler" I think.

Sorry too late. I think build-time is good enough for internal emscripten developers.. but I wanted to be extra explicit in the changelog.

…n-core#20573) This is needed for emscripten-core#20551 so that we can continue to run tests under versions of node that are too old to run the compiler.

…mscripten-core#20551) This new minimum version matches the version that we ship with emsdk. Critically it supports null coalescing & logical assignment needed by emscripten-core#20549.

juj · 2023-11-03T10:43:34Z

I think this is fine for us. Tried to find which OS versions Node.js 16.20.0 LTS supports, though couldn't find info here at least: https://nodejs.org/en/blog/release/v16.20.0

Any chance you might have run into such info while looking at this? (that's fine if not, I can try to hunt deeper myself)

kleisauke · 2023-11-03T10:59:55Z

https://github.com/nodejs/node/blob/v16.x/BUILDING.md#platform-list

Since we updated out min node version to 16 in emscripten-core#20551 we can now assume that `globalThis` (the more standard name) is always available See https://nodejs.org/api/globals.html#global

Since we updated out min node version to 16 in #20551 we can now assume that `globalThis` (the more standard name) is always available See https://nodejs.org/api/globals.html#global

sbc100 requested review from RReverser and kripken October 27, 2023 01:57

RReverser approved these changes Oct 27, 2023

View reviewed changes

sbc100 force-pushed the bump_min_node_version branch 2 times, most recently from 09f14b7 to dfd58d5 Compare October 27, 2023 16:04

sbc100 requested review from RReverser, dschuff and juj October 27, 2023 16:05

sbc100 force-pushed the bump_min_node_version branch from dfd58d5 to 8914c09 Compare October 27, 2023 16:29

sbc100 force-pushed the bump_min_node_version branch 2 times, most recently from e0143f3 to 05116f4 Compare October 27, 2023 17:09

RReverser reviewed Oct 27, 2023

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

RReverser reviewed Oct 27, 2023

View reviewed changes

src/settings.js Show resolved Hide resolved

kripken reviewed Oct 27, 2023

View reviewed changes

sbc100 changed the title ~~Update the minimum supported version of node (10.19.0 -> 16.20.0)~~ Update the minimum supported build-time version of node (10.19.0 -> 16.20.0) Oct 27, 2023

sbc100 changed the title ~~Update the minimum supported build-time version of node (10.19.0 -> 16.20.0)~~ Update the minimum supported version of node to 16.20.0 Oct 27, 2023

sbc100 force-pushed the bump_min_node_version branch 2 times, most recently from 6a55ee5 to 563e23c Compare October 27, 2023 18:03

RReverser approved these changes Oct 27, 2023

View reviewed changes

kripken reviewed Oct 27, 2023

View reviewed changes

RReverser mentioned this pull request Oct 28, 2023

Base support for coalescing & logical assignment #20549

Merged

sbc100 mentioned this pull request Oct 30, 2023

Support separate version of Node.js for running tests. NFC #20573

Merged

sbc100 force-pushed the bump_min_node_version branch from 563e23c to e325726 Compare October 30, 2023 21:16

sbc100 force-pushed the bump_min_node_version branch from e325726 to ad50ab2 Compare October 30, 2023 22:13

sbc100 added a commit that referenced this pull request Oct 30, 2023

Support separate version of Node.js for running tests. NFC (#20573)

bcbfdd4

This is needed for #20551 so that we can continue to run tests under versions of node that are too old to run the compiler.

sbc100 force-pushed the bump_min_node_version branch 2 times, most recently from b49c99f to 8669e6d Compare October 30, 2023 23:35

sbc100 changed the title ~~Update the minimum supported version of node to 16.20.0~~ Update the minimum supported build-time version of node to 16.20.0 Oct 30, 2023

sbc100 force-pushed the bump_min_node_version branch from 8669e6d to 811a9b8 Compare October 30, 2023 23:37

Update the minimum supported build-time version of node to 16.20.0

e6eacab

This new minimum version matches the version that we ship with emsdk. Critically it supports null coalescing & logical assignment needed by emscripten-core#20549.

sbc100 force-pushed the bump_min_node_version branch from 811a9b8 to e6eacab Compare October 30, 2023 23:38

kripken approved these changes Oct 30, 2023

View reviewed changes

sbc100 merged commit eefceb7 into emscripten-core:main Oct 31, 2023

sbc100 deleted the bump_min_node_version branch October 31, 2023 01:04

sbc100 mentioned this pull request Dec 6, 2023

Use globalThis rather than global in JS compiler. NFC #20856

Merged

Update the minimum supported build-time version of node to 16.20.0 #20551

Update the minimum supported build-time version of node to 16.20.0 #20551

Uh oh!

Conversation

sbc100 commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

curiousdannii commented Oct 27, 2023

Uh oh!

RReverser left a comment

Choose a reason for hiding this comment

Uh oh!

RReverser commented Oct 27, 2023

Uh oh!

RReverser commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Oct 27, 2023

Uh oh!

dschuff commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RReverser commented Oct 27, 2023

Uh oh!

sbc100 commented Oct 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Oct 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juj commented Nov 3, 2023

Uh oh!

kleisauke commented Nov 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sbc100 commented Oct 27, 2023 •

edited

Loading

RReverser commented Oct 27, 2023 •

edited

Loading

RReverser commented Oct 27, 2023 •

edited

Loading

dschuff commented Oct 27, 2023 •

edited

Loading