-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
[v14.x] build: pin build-docs workflow to Node.js 14 #40939
Conversation
The build-docs workflow runs tests which means it is sensitive to globals and can fail if the `node` running the test has a different set of globals to that expected by `test/common`.
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.
Another solution would be to backport those checks:
Lines 279 to 301 in d0fa1e9
if (global.AbortController) | |
knownGlobals.push(global.AbortController); | |
if (global.gc) { | |
knownGlobals.push(global.gc); | |
} | |
if (global.performance) { | |
knownGlobals.push(global.performance); | |
} | |
if (global.PerformanceMark) { | |
knownGlobals.push(global.PerformanceMark); | |
} | |
if (global.PerformanceMeasure) { | |
knownGlobals.push(global.PerformanceMeasure); | |
} | |
// TODO(@ethan-arrowood): Similar to previous checks, this can be temporary | |
// until v16.x is EOL. Once all supported versions have structuredClone we | |
// can add this to the list above instead. | |
if (global.structuredClone) { | |
knownGlobals.push(global.structuredClone); | |
} |
I don't think we'd want to allow accidentally introducing new globals into LTS though. |
Fast-track has been requested by @richardlau. Please 👍 to approve. |
The build-docs workflow runs tests which means it is sensitive to globals and can fail if the `node` running the test has a different set of globals to that expected by `test/common`. PR-URL: #40939 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Landed in fb0af94 |
@@ -11,7 +11,7 @@ on: | |||
- v[0-9]+.x | |||
|
|||
env: | |||
NODE_VERSION: lts/* | |||
NODE_VERSION: 14 |
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.
Probably would be good to include a comment so people don't update it to lts/*
subsequently.
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.
Oh, wait, this is on the 14.x-staging branch. Yeah, that (probably) doesn't need a comment.
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 undecided about whether to land something similar on later release lines or master. At least 14 is in maintenance which means we have no plans to introduce new globals.
The build-docs workflow runs tests which means it is sensitive to globals and can fail if the `node` running the test has a different set of globals to that expected by `test/common`. PR-URL: #40939 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
The build-docs workflow runs tests which means it is sensitive to
globals and can fail if the
node
running the test has a differentset of globals to that expected by
test/common
.e.g failure on current
v14.x-staging
https://github.com/nodejs/node/runs/4301335343?check_suite_focus=true
compare to
node/test/common/index.js
Lines 253 to 262 in 0682370