-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
[v22.x] backport module.registerHooks() #57130
base: v22.x-staging
Are you sure you want to change the base?
Conversation
PR-URL: nodejs#55698 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Guy Bedford <[email protected]>
PR-URL: nodejs#55698 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Guy Bedford <[email protected]>
PR-URL: nodejs#56454 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: nodejs#56376 PR-URL: nodejs#56402 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
Review requested:
|
38bdcb8
to
bc0f050
Compare
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.
The diff here is suspiciously HUGE. Are you sure this is correct?
What do you mean by huge? It doesn't appear to be much bigger than the PRs (note that the first PR #55698 has already 2000+ lines added due to the amount of tests added) |
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.
RSLGTM
The only remaining failure seems to be a flake that already exists on the v22.x-staging branch.
See https://ci.nodejs.org/job/node-test-binary-windows-js-suites/33054/RUN_SUBSET=2,nodes=win11-COMPILED_BY-vs2022/testReport/junit/(root)/parallel/test_fs_cp/ from the node-daily-v22.x-staging job today which has the same failure. Should this be merged considering the test failure already exists on v22.x-staging? @nodejs/releasers |
(It may be a pretty bad flake, I am seeing that v22.x-staging has been almost red for most days in the past month now: https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-v22.x-staging/) |
This needs a manual backport because #55698 has some doc-only conflict due to the lack of removal of
--experimental-default-type
(semver-major) in v22.x.I included #57056 which is not yet in v23.x - it will probably be 2 weeks old on v23.x before the next v22.x is out, anyway, so I added it here in case it got forgotten.It might be easier to manage to just leave that out and land this first. That one will just land cleanly if this is already in v22.x-staging.