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.

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

doc,loaders,module: clarify hook chain execution sequence #51884

Merged
merged 5 commits into from Mar 2, 2024
Merged

doc,loaders,module: clarify hook chain execution sequence #51884

merged 5 commits into from Mar 2, 2024

Conversation

JakobJingleheimer
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer commented Feb 26, 2024 •
edited

Closes #51876
Closes #51878

@JakobJingleheimer JakobJingleheimer added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. loaders Issues and PRs related to ES module loaders labels Feb 26, 2024
@JakobJingleheimer JakobJingleheimer self-assigned this Feb 26, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@yklcs
Copy link

yklcs commented Feb 27, 2024

I'd like to suggest making changes in the "hooks" section too for clarity, as it also mentions chaining. The "subsequent loader" part, in specific.

node/doc/api/module.md

Lines 376 to 380 in 6bb7c4d

Hooks are part of a chain, even if that chain consists of only one custom
(user-provided) hook and the default hook, which is always present. Hook
functions nest: each one must always return a plain object, and chaining happens
as a result of each function calling `next<hookName>()`, which is a reference to
the subsequent loader's hook.

GeoffreyBooth
GeoffreyBooth reviewed Feb 27, 2024
View reviewed changes
doc/api/module.md Outdated Show resolved Hide resolved
@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented Feb 27, 2024 •
edited

I'd like to suggest making changes in the "hooks" section too for clarity, as it also mentions chaining. The "subsequent loader" part, in specific.

Hmm, here I'm not seeing the confusing bit. Do you mean you'd like some indication of which loader will be in next<hookName>? I wouldn't want to be too repetitive, so how about something simple, like:

- which is a reference to the subsequent loader's hook.
+ which is a reference to the subsequent loader's hook (in LIFO order).

@yklcs
Copy link

yklcs commented Feb 28, 2024

I'd like to suggest making changes in the "hooks" section too for clarity, as it also mentions chaining. The "subsequent loader" part, in specific.

Hmm, here I'm not seeing the confusing bit here. Do you mean you'd like some indication of which loader will be in next<hookName>? I wouldn't want to be too repetitive, so how about something simple, like:

- which is a reference to the subsequent loader's hook.
+ which is a reference to the subsequent loader's hook (in LIFO order).

I think that would help, maybe along with a link to #chaining?

GeoffreyBooth
GeoffreyBooth reviewed Mar 1, 2024
View reviewed changes
doc/api/module.md Outdated Show resolved Hide resolved
doc/api/module.md Outdated Show resolved Hide resolved
JakobJingleheimer and others added 2 commits March 1, 2024 20:10
@JakobJingleheimer @GeoffreyBooth
wordsmithing
f59a6a8 
Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
@JakobJingleheimer @GeoffreyBooth
loader → hooks
d960816 
Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@JakobJingleheimer

This comment was marked as resolved.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@JakobJingleheimer JakobJingleheimer added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 2, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2e2a848 into nodejs:main Mar 2, 2024
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2e2a848

targos pushed a commit that referenced this pull request Mar 7, 2024
@JakobJingleheimer @targos
doc,module: clarify hook chain execution sequence
20e0ba3 
PR-URL: #51884
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@targos targos mentioned this pull request Mar 7, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@JakobJingleheimer @richardlau
doc,module: clarify hook chain execution sequence
e934278 
PR-URL: #51884
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@JakobJingleheimer @richardlau
doc,module: clarify hook chain execution sequence
90da415 
PR-URL: #51884
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@JakobJingleheimer @rdw-msft
doc,module: clarify hook chain execution sequence
6670927 
PR-URL: nodejs#51884
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@lpinca lpinca lpinca approved these changes

Assignees

@JakobJingleheimer JakobJingleheimer

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

node:module register hook chaining order behavior is incorrect
5 participants
@JakobJingleheimer @nodejs-github-bot @yklcs @GeoffreyBooth @lpinca