esm: fix loader hooks accepting too many arguments by JakobJingleheimer · Pull Request #44109 · nodejs/node

JakobJingleheimer · 2022-08-02T20:24:18Z

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

nodejs-github-bot · 2022-08-02T20:24:22Z

Review requested:

@nodejs/loaders
@nodejs/modules
@cspotcode

cc @targos

cspotcode

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 · 2022-08-02T20:39:49Z

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

account for prototype pollution in Array & Object

consolidate assignment & set arg0 default

validateArgs array → separate args

…arguments ReflectApply → direct invocation w separate args

nodejs-github-bot · 2022-08-03T19:39:08Z

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

nodejs-github-bot · 2022-08-03T22:46:18Z

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

nodejs-github-bot · 2022-08-04T05:28:44Z

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

nodejs-github-bot · 2022-08-04T08:39:08Z

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 <3012099+JakobJingleheimer@users.noreply.github.com>
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

JakobJingleheimer · 2022-08-04T08:42:32Z

✖ This PR needs to wait 11 more hours to land

Grr

nodejs-github-bot · 2022-08-04T22:10:14Z

Landed in e0b440a

PR-URL: #44109 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs/node#44109 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

esm: fix loader hooks accepting too many arguments
2f1d595

JakobJingleheimer added the loaders Issues and PRs related to ES module loaders label Aug 2, 2022

JakobJingleheimer added this to the ESM Loaders: Stable (CJS parity) milestone Aug 2, 2022

JakobJingleheimer requested review from GeoffreyBooth and aduh95 August 2, 2022 20:24

nodejs-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 reviewed Aug 2, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/loader.js Outdated

GeoffreyBooth approved these changes Aug 2, 2022
View reviewed changes

aduh95 reviewed Aug 2, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/loader.js Outdated
Comment threadlib/internal/modules/esm/loader.js Outdated

guybedford approved these changes Aug 3, 2022
View reviewed changes

fixup! esm: fix loader hooks accepting too many arguments
e14ce33
account for prototype pollution in Array & Object

JakobJingleheimer commented Aug 3, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/loader.js

aduh95 reviewed Aug 3, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/loader.js Outdated
Comment threadlib/internal/modules/esm/loader.js Outdated
Comment threadlib/internal/modules/esm/loader.js Outdated

JakobJingleheimer added 3 commits August 3, 2022 20:45

fixup! fixup! esm: fix loader hooks accepting too many arguments
b3d1461
consolidate assignment & set arg0 default

fixup! fixup! fixup! esm: fix loader hooks accepting too many arguments
380c9a5
validateArgs array → separate args

fixup! fixup! fixup! fixup! esm: fix loader hooks accepting too many …
2254fb8
…arguments ReflectApply → direct invocation w separate args

aduh95 reviewed Aug 3, 2022
View reviewed changes

Comment threadtest/es-module/test-esm-loader-chaining.mjs Outdated

aduh95 reviewed Aug 3, 2022
View reviewed changes

Comment threadtest/es-module/test-esm-loader-chaining.mjs Outdated

improve substring match
1eff091

aduh95 approved these changes Aug 3, 2022
View reviewed changes

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022

aduh95 reviewed Aug 3, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/loader.js Outdated

aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 3, 2022

correct context arg passed to nextHook fn
fad4ccf

JakobJingleheimer added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022

GeoffreyBooth approved these changes Aug 3, 2022
View reviewed changes

JakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022

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

MoLow 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-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022

nodejs-github-bot merged commit e0b440a into nodejs:mainAug 4, 2022

github-actionsBot mentioned this pull request Aug 5, 2022
CI Reliability 2022-08-05 nodejs/reliability#337
Open
17 tasks

cspotcode mentioned this pull request Aug 5, 2022
when calling default* functions, you should not recursively pass default* function TypeStrong/ts-node#1849
Closed

github-actionsBot mentioned this pull request Aug 6, 2022
CI Reliability 2022-08-06 nodejs/reliability#338
Open
16 tasks

alichry mentioned this pull request Aug 6, 2022
Using testdouble esm loader with ts-node/esm loader on mocha leads to errors (Node 18.6.0) testdouble/testdouble.js#496
Closed

targos mentioned this pull request Aug 6, 2022
v16.17.0 release proposal #44098
Merged

This was referenced Aug 7, 2022
CI Reliability 2022-08-07 nodejs/reliability#339
Open
CI Reliability 2022-08-08 nodejs/reliability#340
Open

danielleadams mentioned this pull request Aug 16, 2022
v18.8.0 release proposal #44246
Closed

ruyadorno mentioned this pull request Aug 23, 2022
v18.8.0 release proposal #44353
Merged

Uh oh!

Uh oh!

Conversation

JakobJingleheimer commented Aug 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 2, 2022• edited by JakobJingleheimer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cspotcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cspotcode commented Aug 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 3, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 3, 2022

Uh oh!

nodejs-github-bot commented Aug 4, 2022

Uh oh!

nodejs-github-bot commented Aug 4, 2022

Uh oh!

JakobJingleheimer commented Aug 4, 2022

Uh oh!

nodejs-github-bot commented Aug 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JakobJingleheimer commented Aug 2, 2022•
edited
Loading

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