esm: change transformSource to load for error

[bmacnaughton]

The error message in stringify() still refers to transformSource as the loader.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[GeoffreyBooth]

Why is this targeting v20.x-staging rather than main?

[JakobJingleheimer]

Should a test case be added to cover this scenario?

[bmacnaughton]

Why is this targeting v20.x-staging rather than main?

Because I didn't double check this comment. It should be against main because it's still incorrect in v21.

Should I close this and reopen another PR or can you change the target?

[bmacnaughton]

Should a test case be added to cover this scenario?

I don't think so; the transformSource() hook no longer exists so this is nothing more than a documentation change.

[JakobJingleheimer]

The modified file (lib/internal/modules/esm/translators.js) is not a doc file—it's part of the implementation (ps I think the subsystem in the PR title is wrong?)

[bmacnaughton]

The modified file (lib/internal/modules/esm/translators.js) is not a doc file—it's part of the implementation (ps I think the subsystem in the PR title is wrong?)

Fair enough; I went back and forth on what subsystem to use. I don't have enough experience with node PRs to know when it's a doc (even as an error message) and when it's the subsystem (changes the flow or logic). That was my criteria for the decision of what subsystem to use; I can't say it was right. I can change that commit message if it should be "esm:" or "module:" (I've seen both in commit messages for that file. I've also seen "doc:" for fixing the spelling of "WebAssembly")

Still, what is there to test? The message either contains the name of a non-existent hook or it doesn't (like the other instances of the same error in the same file).

[JakobJingleheimer]

Fair enough; I went back and forth on what subsystem to use. I don't have enough experience with node PRs to know when it's a doc (even as an error message) and when it's the subsystem (changes the flow or logic). That was my criteria for the decision of what subsystem to use; I can't say it was right. I can change that commit message if it should be "esm:" or "module:" (I've seen both in commit messages for that file.

Ahh, very understandable 🙂

doc is for anything within the doc directory (md files).

This would be either esm or module. The difference here splits hairs.

The test would do exactly as you describe: verify the hook name within the error message is correct. We have many such test cases 🙂 they use a regex match against the error message and assert certain substrings are found. See test-esm-loader-hooks.mjs (which may or may not be the appropriate suite to add a test for this particular issue).

[bmacnaughton]

Any feedback on the test would be appreciated. I wasn't sure whether it made sense to match on the entire message or just the substring that referred to the function name; I went with the more strict check.

Also, It seems like tools/test.py should be able to run a single suite but that's not the way it works right now. I've made this change locally, but could either add it to this PR or create a new PR, if I'm not misinterpreting what it is supposed to do.

$ git diff
diff --git a/tools/test.py b/tools/test.py
index d35b45a669..d7854fdf6c 100755
--- a/tools/test.py
+++ b/tools/test.py
@@ -1617,7 +1617,8 @@ def Main():
     test_root = options.test_root
   suites = GetSuites(test_root)
   repositories = [TestRepository(join(test_root, name)) for name in suites]
-  repositories += [TestRepository(a) for a in options.suite]
+  if options.suite:
+    repositories = [TestRepository(a) for a in options.suite]
 
   root = LiteralTestSuite(repositories, test_root)
   paths = ArgsToTestPaths(test_root, args, suites)

It would also be possible to find the suites and teach the args parser to accept only those (though maybe the test-root option could change that). It might make sense to add a little more to the help and/or docs to note that suites have to specify the relative path, e.g., test/es-module, not just es-module. These aren't a big deal, but they are things that had to be discovered by a newbie.

[aduh95]

It seems like tools/test.py should be able to run a single suite but that's not the way it works right now.

Can you clarify what you mean? Doesn’t tools/test.py test/es-modules run only the esm suite for you?
Anyway, improvements to the test runner should definitely be done on in a separate PR.

[bmacnaughton]

Can you clarify what you mean? Doesn’t tools/test.py test/es-modules run only the esm suite for you?

Yes, that works. I misunderstood test.py's help - I thought --suite was the way to specify a single suite. Help didn't mention arguments and I wasn't looking for that that in the code. --suite is apparently intended to add an additional suite.

[bmacnaughton]

that's not strictly in-scope for this PR, so you could toss that back.

I'm not a big fan of strictly-in-scope. I could change the name of the PR if that's important; fixing the error message was the broad goal. I hope you're OK with that. And "hook" is far more precise than "function" - it was a good catch.

[GeoffreyBooth]

I’m not a big fan of strictly-in-scope.

If you’re okay with it, I think changing two words within the same error message is certainly related enough to be done in one PR.

[bmacnaughton]

I made changes to fix my test to return an invalid source that isn't null. The change detects a null source, so the original test will no longer succeed.

Should there be a new test to verify that ENOENT can be returned when source is null/undefined?

[aduh95]

The test returns ENOENT only because you're using a URL that does not match a real file, so it's a bit orthogonal to the issue (and we would need to use a file path that we are 100% confident would not exist on the test machine to avoid false negative). I don't think it's necessary to add such test in this PR.

[nodejs-github-bot]

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

[bmacnaughton]

it's pretty minor, but should "source" and 'load' both use the same single/double quotation? i guess it's not really possible given that the name now contains the filename containing the hook.

[aduh95]

it's pretty minor, but should "source" and 'load' both use the same single/double quotation? i guess it's not really possible given that the name now contains the filename containing the hook.

If someone cares to open a PR for that, it would probably be accepted; it doesn't bother me personally.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/50466
✔  Done loading data for nodejs/node/pull/50466
----------------------------------- PR info ------------------------------------
Title      esm: change transformSource to load for error (#50466)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     bmacnaughton:v20.x-staging -> nodejs:main
Labels     esm, author ready, needs-ci, loaders, commit-queue-squash
Commits    8
 - esm: change transformSource to load for error
 - add test for "load" name
 - Merge branch 'nodejs:main' into main
 - shorten line with regex
 - address PR feedback
 - change to assert.match()
 - fix test
 - Update test/es-module/test-esm-loader.mjs
Committers 2
 - bmacnaughton 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/50466
Reviewed-By: Geoffrey Booth 
Reviewed-By: Jacob Smith 
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50466
Reviewed-By: Geoffrey Booth 
Reviewed-By: Jacob Smith 
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 29 Oct 2023 12:50:21 GMT
   ✔  Approvals: 4
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/50466#pullrequestreview-1714235735
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/50466#pullrequestreview-1703633126
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/50466#pullrequestreview-1726084935
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/50466#pullrequestreview-1765849968
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-11-30T12:52:35Z: https://ci.nodejs.org/job/node-test-pull-request/56014/
- Querying data for job/node-test-pull-request/56014/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 50466
From https://github.com/nodejs/node
 * branch                  refs/pull/50466/merge -> FETCH_HEAD
✔  Fetched commits as 27b2ce5ba633..934406adfc12
--------------------------------------------------------------------------------
Auto-merging lib/internal/modules/esm/translators.js
[main e412bc337f] esm: change transformSource to load for error
 Author: bmacnaughton 
 Date: Mon Oct 30 05:03:49 2023 -0700
 1 file changed, 1 insertion(+), 1 deletion(-)
error: commit 92bff8e3ef96085cf787bd6bbfd8d9eb700ab0c7 is a merge but no -m option was given.
fatal: cherry-pick failed
[main 52bb4ce70e] add test for "load" name
 Author: bmacnaughton 
 Date: Sun Nov 5 03:28:55 2023 -0800
 2 files changed, 16 insertions(+)
   ✘  Failed to apply patches

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

[aduh95]

Landed in ab857e1, thanks for the contribution 🎉