Skip to content

[v22.x] backport module.registerHooks() #57130

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[v22.x] backport module.registerHooks() #57130

wants to merge 4 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 18, 2025 •
edited
Loading

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.

joyeecheung and others added 4 commits February 18, 2025 22:58
@joyeecheung
module: implement module.registerHooks()
cd50d3f 
PR-URL: nodejs#55698
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@joyeecheung
module: use synchronous hooks for preparsing in import(cjs)
49d7c2e 
PR-URL: nodejs#55698
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@JakobJingleheimer @joyeecheung
doc: correct customization hook types & clarify descriptions
6564c7e 
PR-URL: nodejs#56454
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@awto @joyeecheung
module: fixing url change in load sync hook chain
bc0f050 
Fixes: nodejs#56376
PR-URL: nodejs#56402
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Feb 18, 2025
@joyeecheung joyeecheung requested a review from a team February 18, 2025 22:07
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65306/

JakobJingleheimer
JakobJingleheimer reviewed Feb 27, 2025
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a 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?

@JakobJingleheimer JakobJingleheimer mentioned this pull request Feb 27, 2025
Merged
@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 2, 2025 •
edited
Loading

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)

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65507/

mcollina
mcollina approved these changes Mar 2, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65544/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65589/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65607/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65613/

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 7, 2025 •
edited
Loading

The only remaining failure seems to be a flake that already exists on the v22.x-staging branch.

15:09:01 not ok 306 parallel/test-fs-cp
15:09:01   ---
15:09:01   duration_ms: 666.01900
15:09:01   severity: fail
15:09:01   exitcode: 1
15:09:01   stack: |-
15:09:01     node:assert:128
15:09:01       throw new AssertionError(obj);
15:09:01       ^
15:09:01     
15:09:01     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
15:09:01     + actual - expected
15:09:01     
15:09:01     + 'ERR_FS_EISDIR'
15:09:01     - 'ERR_FS_CP_EINVAL'
15:09:01               ^
15:09:01     
15:09:01         at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp.mjs:687:12
15:09:01         at c:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:435:15
15:09:01         at node:fs:188:23
15:09:01         at callbackifyOnRejected (node:util:374:10)
15:09:01         at process.processTicksAndRejections (node:internal/process/task_queues:90:21) {
15:09:01       generatedMessage: true,
15:09:01       code: 'ERR_ASSERTION',
15:09:01       actual: 'ERR_FS_EISDIR',
15:09:01       expected: 'ERR_FS_CP_EINVAL',
15:09:01       operator: 'strictEqual'
15:09:01     }
15:09:01     
15:09:01     Node.js v22.14.1-pre
15:09:01   ...

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

@joyeecheung
Copy link
Member Author

(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/)

@StefanStojanovic
Copy link
Contributor

This test is already marked flaky on main in 304bb9c.

As seen from this comment the fix for this already exists in libuv and will make its way into Node at some point. Until then, it would probably make sense to backport the commit mentioned above in all LTS branches.

@richardlau
Copy link
Member

As seen from this comment the fix for this already exists in libuv and will make its way into Node at some point. Until then, it would probably make sense to backport the commit mentioned above in all LTS branches.

@StefanStojanovic FWIW libuv updates in Node.js 22 (and presumably earlier release lines) are blocked on CI failures on 32-bit Windows: #57316

@RafaelGSS
Copy link
Member

Hi @joyeecheung, just a FYI, I'll issue a v22 release soon this week, and this PR might not go due to red-CI.

@joyeecheung
Copy link
Member Author

@RafaelGSS The last time the CI was run #57130 (comment) the failures were the same failures already failing v22.x-staging. If v22.x-staging is green then this should be green. Though I don't know if it's necessary to restart a CI to check again.

@RafaelGSS
Copy link
Member

Ok, I can land it on v22.x-staging and we can check on proposal if an error pops-up related to this PR (which is unlikely)

RafaelGSS pushed a commit that referenced this pull request Mar 31, 2025
@joyeecheung @RafaelGSS
module: implement module.registerHooks()
b2e44a8 
PR-URL: #55698
Backport-PR-URL: #57130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Mar 31, 2025
@joyeecheung @RafaelGSS
module: use synchronous hooks for preparsing in import(cjs)
6b93ba7 
PR-URL: #55698
Backport-PR-URL: #57130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Mar 31, 2025
@JakobJingleheimer @RafaelGSS
doc: correct customization hook types & clarify descriptions
eff4295 
PR-URL: #56454
Backport-PR-URL: #57130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Mar 31, 2025
@awto @RafaelGSS
module: fixing url change in load sync hook chain
926b887 
Fixes: #56376
PR-URL: #56402
Backport-PR-URL: #57130
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS
Copy link
Member

Landed in a68f127...926b887

@RafaelGSS RafaelGSS closed this Mar 31, 2025
@parksb parksb mentioned this pull request Apr 24, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@joyeecheung @nodejs-github-bot @StefanStojanovic @richardlau @RafaelGSS @mcollina @JakobJingleheimer @awto