GeezerVison

Review requested:

• @nodejs/loaders

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

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?

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.

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?)

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).

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).

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.

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.

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.

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.

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.

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?

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.

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

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.

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.

Landed in ab857e1, thanks for the contribution 🎉