Skip to content
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

esm: fix register hook chaining behavior #51878

Closed
wants to merge 1 commit into from

Conversation

yklcs
Copy link

@yklcs yklcs commented Feb 26, 2024

Chains module register hooks in order of registration. Fixes: #51876

Chains module register hooks in order of registration.
Fixes: nodejs#51876
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Feb 26, 2024
Comment on lines -149 to +150
const moduleLoader = require('internal/process/esm_loader').esmLoader;
const moduleLoader = createModuleLoader();
Copy link
Member

Choose a reason for hiding this comment

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

Was this change necessary for it to work? The existing code was done this way for a reason (to reduce node's startup time by loading the dependency just in time). You'll see that done in many places within this file.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

This needs tests that fail on current main and pass here.

@yklcs
Copy link
Author

yklcs commented Feb 26, 2024

This PR is not required since LIFO behavior was actually intended, see #51876. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node:module register hook chaining order behavior is incorrect
4 participants