esm: remove erroneous context.parentURL property passed to load hook

[JakobJingleheimer]

parentURL was accidentally added to custom load hook's context in #36328 due to a bad merge conflict resolution.

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[nodejs-github-bot]

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

[bmeck]

cc: @nodejs/loaders

[aduh95]

I don't find mod very descriptive (if it's a well known acronym that I don't know, please ignore)

Suggested change

	import mod from 'internal/modules/esm/loader';
	import loader from 'internal/modules/esm/loader';

[JakobJingleheimer]

It's not super common. I usually use module, but some linters freak out when they find it in an ESM file. I considered loader, but it's not the loader, so I opted for mod

[JakobJingleheimer]

I went with esmLoaderModule (not married to it).

[GeoffreyBooth]

I dislike default exports in general. As far as I’m concerned, esm/loader doesn’t need a default export and you should be able to just import it via import { loader } from 'internal/modules/esm/loader'.

[JakobJingleheimer]

Yeah, that's what I wanted to do. It doesn't work: #41975 (comment)

This was kind of weird btw: esm/loader.js exports ESMLoader as a prop on exports

node/lib/internal/modules/esm/loader.js

Line 589 in bbd7cf7

exports.ESMLoader = ESMLoader;

So I expect it to be exposed as a named export (thus precluding this altogether).

esmLoaderModule is an object that contains a property named ESMLoader; there are no named exports available :(

[GeoffreyBooth]

Why this big block?

[JakobJingleheimer]

😅 #41975 (review)

[GeoffreyBooth]

Okay, but until you have the other cases it doesn’t make much sense to have just one block. I would also argue that perhaps other cases should be in new files, as this one’s already pretty large.

[JakobJingleheimer]

I was thinking it would make sense to keep them in the same file. There are already a lot of files in this directory, many of which SEEM redundant (which makes it difficult to figure out what might or should be where). If the file gets renamed at the same time as more cases are added, git will likely break the history.

I could see removing the block for now and add it back when the other cases are added.

I would prefer to keep them in the same file though, unless they differ a lot or become unruly. Could I persuade you to wait and see?

[GeoffreyBooth]

I’ve already approved, so you can do whatever. Maybe just add a comment at the top of the block saying this is here because you anticipate adding more blocks in the future, and what this block is meant to test.

[GeoffreyBooth]

Can you make it clear somewhere that what you’re testing in this file is the signatures of the hooks? Like maybe name the file something like test-esm-loader-hooks-signatures.mjs, and add some comments near these assertions that what we’re doing here is testing that the expected arguments/properties exist and that unexpected ones don’t exist?

And I assume that’s all you’re aiming to test in this file?

[JakobJingleheimer]

Can you make it clear somewhere that what you’re testing in this file is the signatures of the hooks? […] add some comments near these assertions that what we’re doing here is testing that the expected arguments/properties exist and that unexpected ones don’t exist?

Sure!

And I assume that’s all you’re aiming to test in this file?

No, I'm planning (in the near future) to add more tests for hooks themselves in this file (hence the generic file name). Those aren't relevant to this PR, so I didn't add them yet.

[GeoffreyBooth]

It’s much easier to debug failing tests when each test file is as close as possible to one test per file, since you can re-run a test file like ./node test/es-module/test-esm-loader-hooks.mjs etc. The loader hooks API is large enough that I can’t imagine testing all of it in one file; you could make a subfolder for it perhaps.

[aduh95]

The test already pass on master, can you add a test case that calls esmLoader.getModuleJob() that doesn't pass on master but passes with this change please?

[JakobJingleheimer]

@aduh95 oh wow. good catch. The reason for that is assert.deepStrictEqual() apparently does not work on arrays:

assert.deepStrictEqual(['foo', 'bar'], ['foo']); // passes 🤯

Is there a way to get it to behave as expected? Otherwise, I might go with:

assert.strictEqual(expected.length, actual.length);
assert.ok(expected.every((v, i) => v === actual[i]));

[aduh95]

The reason for that is assert.deepStrictEqual() apparently does not work on arrays:

assert.deepStrictEqual(['foo', 'bar'], ['foo']); // passes 🤯

That doesn't seem to be correct:

$ node -p "assert.deepStrictEqual(['foo', 'bar'], ['foo'])"
node:assert:123
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
    'foo',
+   'bar'
  ]
    at [eval]:1:8
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:75:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:74:60)
    at node:internal/main/eval_string:27:3 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: [ 'foo', 'bar' ],
  expected: [ 'foo' ],
  operator: 'deepStrictEqual'
}

I think the issue is this PR changes code in the getModuleJob method, but the test never calls it.

[JakobJingleheimer]

Ah, yes, sorry: I changed the test to call getModuleJob(), but it still passes.

When I put that foo bar assert.deepStrictEquals in the test, it passes. The alternative one fails (as it should).

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/41975
✔  Done loading data for nodejs/node/pull/41975
----------------------------------- PR info ------------------------------------
Title      esm: remove erroneous `context.parentURL` property passed to `load` hook (#41975)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     JakobJingleheimer:fix/remove-erroneous-parentURL-from-load-hook-context -> nodejs:master
Labels     esm, author ready, needs-ci, loaders
Commits    4
 - esm: remove erroneous `context.parentURL` property passed to `load` hook
 - fixup: ensure custom hooks are called & clarify import name
 - fixup: add comments explaining purpose of new test set
 - fixup: avoid undesirably bypassing implementation
Committers 1
 - Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/41975
Reviewed-By: Geoffrey Booth 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41975
Reviewed-By: Geoffrey Booth 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 14 Feb 2022 21:07:11 GMT
   ✔  Approvals: 3
   ✔  - Geoffrey Booth (@GeoffreyBooth): https://github.com/nodejs/node/pull/41975#pullrequestreview-884858704
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41975#pullrequestreview-887153422
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/41975#pullrequestreview-887155214
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-18T17:24:46Z: https://ci.nodejs.org/job/node-test-pull-request/42663/
- Querying data for job/node-test-pull-request/42663/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
   68c4b8d56d..6700b5c9e1  master     -> origin/master
✔  origin/master is now up-to-date
master is out of sync with origin/master. Mismatched commits:
 - e2a5667af9 tools: fix bugs in prefer-primordials linter rule
 - 6700b5c9e1 tools: fix bugs in prefer-primordials linter rule
--------------------------------------------------------------------------------
HEAD is now at 6700b5c9e1 tools: fix bugs in prefer-primordials linter rule
   ✔  Reset to origin/master
- Downloading patch for 41975
From https://github.com/nodejs/node
 * branch                  refs/pull/41975/merge -> FETCH_HEAD
✔  Fetched commits as 6700b5c9e114..f39a2ca79965
--------------------------------------------------------------------------------
[master 7d2ca318bd] esm: remove erroneous `context.parentURL` property passed to `load` hook
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Mon Feb 14 21:59:05 2022 +0100
 2 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 test/es-module/test-esm-loader-hooks.mjs
[master a238a7e612] fixup: ensure custom hooks are called & clarify import name
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Tue Feb 15 21:15:03 2022 +0100
 1 file changed, 73 insertions(+), 70 deletions(-)
 rewrite test/es-module/test-esm-loader-hooks.mjs (67%)
[master 5b81a6f031] fixup: add comments explaining purpose of new test set
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Wed Feb 16 19:13:11 2022 +0100
 1 file changed, 7 insertions(+), 1 deletion(-)
[master 1f0d95955f] fixup: avoid undesirably bypassing implementation
 Author: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
 Date: Thu Feb 17 14:18:12 2022 +0100
 1 file changed, 11 insertions(+), 14 deletions(-)
   ✔  Patches applied
There are 4 commits in the PR. Attempting autorebase.
Rebasing (2/8)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

esm: remove erroneous context.parentURL property passed to load hook
PR-URL: #41975

Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
[detached HEAD 334cdddcd8] esm: remove erroneous context.parentURL property passed to load hook

Author: Jacob Smith 3012099+JakobJingleheimer@users.noreply.github.com

Date: Mon Feb 14 21:59:05 2022 +0100

2 files changed, 70 insertions(+), 1 deletion(-)

create mode 100644 test/es-module/test-esm-loader-hooks.mjs

Rebasing (3/8)

Rebasing (4/8)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup: ensure custom hooks are called & clarify import name

rename mod → esmLoaderModule

PR-URL: #41975

Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
[detached HEAD 4b1789d9fa] fixup: ensure custom hooks are called & clarify import name

Author: Jacob Smith 3012099+JakobJingleheimer@users.noreply.github.com

Date: Tue Feb 15 21:15:03 2022 +0100

1 file changed, 73 insertions(+), 70 deletions(-)

rewrite test/es-module/test-esm-loader-hooks.mjs (67%)

Rebasing (5/8)

Rebasing (6/8)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup: add comments explaining purpose of new test set
PR-URL: #41975

Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
[detached HEAD 3e8ea43a42] fixup: add comments explaining purpose of new test set

Author: Jacob Smith 3012099+JakobJingleheimer@users.noreply.github.com

Date: Wed Feb 16 19:13:11 2022 +0100

1 file changed, 7 insertions(+), 1 deletion(-)

Rebasing (7/8)

Rebasing (8/8)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup: avoid undesirably bypassing implementation
PR-URL: #41975

Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
[detached HEAD 39adab15bf] fixup: avoid undesirably bypassing implementation

Author: Jacob Smith 3012099+JakobJingleheimer@users.noreply.github.com

Date: Thu Feb 17 14:18:12 2022 +0100

1 file changed, 11 insertions(+), 14 deletions(-)
Successfully rebased and updated refs/heads/master.
ℹ  Use --fixupAll option, squash the PR manually or land the PR from the command line.

https://github.com/nodejs/node/actions/runs/1866011121

[nodejs-github-bot]

Landed in 8843381