Skip to content

esm: fix loader hooks accepting too many arguments#44109

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
Merged
nodejs-github-bot merged 7 commits into from
Aug 4, 2022
Merged

esm: fix loader hooks accepting too many arguments #44109

nodejs-github-bot merged 7 commits into from
Aug 4, 2022

Conversation

@JakobJingleheimer
Copy link
Member

@JakobJingleheimerJakobJingleheimer commented Aug 2, 2022•
edited
Loading

When a user supplies too many arguments, those beyond the 2nd arg are currently blindly passed to the next<HookName> function. Now, those extra args are ignored. It assumes both resolve and load share the same number of args—which they currently do; if that changes, this will need to be updated.

Fixes#44108

@JakobJingleheimerJakobJingleheimer added the loaders Issues and PRs related to ES module loaders label Aug 2, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 2, 2022•
edited by JakobJingleheimer
Loading

Review requested:

cc @targos

@nodejs-github-botnodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Aug 2, 2022
cspotcode
cspotcode reviewed Aug 2, 2022
View reviewed changes
Copy link

@cspotcodecspotcode left a comment

Choose a reason for hiding this comment

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

Looks good to me. Github won't let me mark this review as "Approve" so I submitted it as "Comment" instead. I asked a question but it's not a blocker.

@cspotcode
Copy link

cspotcode commented Aug 2, 2022

It assumes both resolve and load share the same number of args—which they currently do; if that changes, this will need to be updated.

Noting for completeness: the code before this pull request also assumed 2 args, so this PR is not introducing an additional limitation or restriction; it was already like that. The switch-case did not check if args.length >= 2; it checked args.length === 2

aduh95
aduh95 reviewed Aug 2, 2022
View reviewed changes
@JakobJingleheimer
fixup! esm: fix loader hooks accepting too many arguments
e14ce33
account for prototype pollution in Array & Object
JakobJingleheimer
JakobJingleheimer commented Aug 3, 2022
View reviewed changes
aduh95
aduh95 reviewed Aug 3, 2022
View reviewed changes
aduh95
aduh95 reviewed Aug 3, 2022
View reviewed changes
aduh95
aduh95 reviewed Aug 3, 2022
View reviewed changes
@aduh95aduh95 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 3, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 3, 2022

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

aduh95
aduh95 reviewed Aug 3, 2022
View reviewed changes
@aduh95aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 3, 2022
@JakobJingleheimerJakobJingleheimer added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 3, 2022

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 4, 2022

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

@JakobJingleheimerJakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 4, 2022

Commit Queue failed
- Loading data for nodejs/node/pull/44109 ✔ Done loading data for nodejs/node/pull/44109 ----------------------------------- PR info ------------------------------------ Title esm: fix loader hooks accepting too many arguments (#44109) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch JakobJingleheimer:fix/esm-loader-accepts-too-many-args -> nodejs:main Labels esm, needs-ci, loaders, commit-queue-squash Commits 7 - esm: fix loader hooks accepting too many arguments - fixup! esm: fix loader hooks accepting too many arguments - fixup! fixup! esm: fix loader hooks accepting too many arguments - fixup! fixup! fixup! esm: fix loader hooks accepting too many arguments - fixup! fixup! fixup! fixup! esm: fix loader hooks accepting too many … - improve substring match - correct context arg passed to nextHook fn Committers 1 - Jacob Smith <[email protected]> PR-URL: https://github.com/nodejs/node/pull/44109 Reviewed-By: Geoffrey Booth Reviewed-By: Guy Bedford Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/44109 Reviewed-By: Geoffrey Booth Reviewed-By: Guy Bedford Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 02 Aug 2022 20:24:18 GMT ✔ Approvals: 3 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/44109#pullrequestreview-1061107695 ✔ - Guy Bedford (@guybedford): https://github.com/nodejs/node/pull/44109#pullrequestreview-1059598743 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/44109#pullrequestreview-1060885869 ✖ This PR needs to wait 11 more hours to land ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-08-04T05:28:44Z: https://ci.nodejs.org/job/node-test-pull-request/45830/ - Querying data for job/node-test-pull-request/45830/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2795302869

@nodejs-github-botnodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 4, 2022
@JakobJingleheimer
Copy link
MemberAuthor

JakobJingleheimer commented Aug 4, 2022

✖ This PR needs to wait 11 more hours to land

Grr

@MoLowMoLow added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 4, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022
@nodejs-github-botnodejs-github-bot merged commit e0b440a into nodejs:mainAug 4, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 4, 2022

Landed in e0b440a

@github-actionsgithub-actionsbot mentioned this pull request Aug 5, 2022
Open
17 tasks
@cspotcodecspotcode mentioned this pull request Aug 5, 2022
Closed
@github-actionsgithub-actionsbot mentioned this pull request Aug 6, 2022
Open
16 tasks
@alichryalichry mentioned this pull request Aug 6, 2022
Closed
targos pushed a commit that referenced this pull request Aug 6, 2022
@JakobJingleheimer@targos
esm: fix loader hooks accepting too many arguments
ae157d9
PR-URL: #44109 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@targostargos mentioned this pull request Aug 6, 2022
Merged
This was referenced Aug 7, 2022
Open
Open
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
@JakobJingleheimer@danielleadams
esm: fix loader hooks accepting too many arguments
c2ea22f
PR-URL: #44109 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Aug 16, 2022
Closed
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
@JakobJingleheimer@ruyadorno
esm: fix loader hooks accepting too many arguments
5e9c197
PR-URL: #44109 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@ruyadornoruyadorno mentioned this pull request Aug 23, 2022
Merged
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@JakobJingleheimer@Fyko
esm: fix loader hooks accepting too many arguments
c9b3809
PR-URL: nodejs#44109 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@JakobJingleheimer@guangwong
esm: fix loader hooks accepting too many arguments
e06f081
PR-URL: nodejs/node#44109 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 approved these changes

@GeoffreyBoothGeoffreyBoothGeoffreyBooth approved these changes

@guybedfordguybedfordguybedford approved these changes

+1 more reviewer

@cspotcodecspotcodecspotcode left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.esmIssues and PRs related to the ECMAScript Modules implementation.loadersIssues and PRs related to ES module loadersneeds-ciPRs that need a full CI run.

Projects

None yet

Milestone

ESM Loaders: Stable (CJS parity)

Development

Successfully merging this pull request may close these issues.

Loader hooks' next functions behave unexpectedly when arguments.length is longer, even if documented named arguments are correct

7 participants

@JakobJingleheimer@nodejs-github-bot@cspotcode@GeoffreyBooth@guybedford@aduh95@MoLow