-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Bump minimum supported node version 10.19.0 -> 12.22.9 #24807
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
base: main
Are you sure you want to change the base?
Conversation
# Keep this in sync with the test-node-compat in .circleci/config.yml. | ||
OLDEST_SUPPORTED_NODE = 101900 | ||
OLDEST_SUPPORTED_NODE = 122209 # Janurary 11, 2022 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to keep node so much more recent (2022) than the browsers (2016)?
Or, is there a particular benefit to bumping node from 10 to 12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to remove the complications around the --experimental-wasm-bigint
flag. Right now we have to worry about 3 different cases:
- Really old versions of node that don't support that flag
- Somewhat old version that support the feature but require the flag
- New versions don't support that flag because its now the default.
But bumping to 12 we no longer need to worry bout the first case which simplifies things somewhat.
Also, I don't think we have any reason to support older versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to keep node so much more recent (2022) than the browsers (2016)?
I think the reason here is that folks who run servers don't tend to let them get quite that out of date. I don't think we have any server-side users who are running such ancient distros.. but I could be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my concern is that I feel I know more about web users than node users... I'm really not sure what kind of node versions people use. And 2022 feels fairly recent to me, though that's subjective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth asking on the mailing list before making this change?
671f889
to
c8a7c59
Compare
# This allows much of the test suite to be run on older versions of node that don't | ||
# support wasm bigint integration | ||
if node_version[0] < feature_matrix.min_browser_versions[feature_matrix.Feature.JS_BIGINT_INTEGRATION]['node'] / 10000: | ||
self.cflags.append('-sWASM_BIGINT=0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm particularly interested in delete this gross couple of lines.. here we are basically saying that if the node version we are testing against is old enough we change the default settings for -sWASM_BIGINT
... which seems very hacky.
This is the minimum version that we support for running the generated code. The version is old enough to cover both Ubuntu LTS (22.04) (Jammy) and Debian/stable (bookworm).
c8a7c59
to
db77efe
Compare
This is the minimum version that we support for running the generated code. The version is old enough to cover both Ubuntu LTS (22.04) (Jammy) and Debian/stable (bookworm).