esm: avoid throw when module specifier is not url

[CraigMacomber]

This particular exception is responsible for a lot of overhead when debugging with large esm codebases with VS Code and break on caught exceptions is enabled.

VS Code silently suppresses this exception, but the mechanism it uses to do so is a bit slow so avoiding this common exception can speed up loading of esm code.

In my scenario (running a single test from a particular test suite un VS Code's debugger) this saved over half of the total run time (over 20 seconds) bring the runtime from over 40 seconds to around 17 (Timing isn't too consistent as it hits a bunch of exceptions I have to continue through along the way).

This should also make debugging without suppression of this exception more pleasant in other tools such as the Chrome dev tools when attached to NodeJs processes.

This is my first attempt at contributing to NodeJS, so I don't know the conventions here very well. If it is the case that this codebase prefers the approach of throwing and catching exceptions in cases like these, and thus does not want to do this change to benefit a specific usage pattern by a third-party tool, that is understandable and, in that case, feel free to just close this PR. This change was simply so easy that it was easier to just propose it as a PR than to ask if this kind of change would be accepted.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[JakobJingleheimer]

This seems reasonable to me

[Renegade334]

Out of interest, you never linked the exact test case – is it genuinely hammering invalid import attempts?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.52%. Comparing base (83ba6b1) to head (95c0855).
⚠️ Report is 93 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #61000   +/-   ##
=======================================
  Coverage   88.51%   88.52%           
=======================================
  Files         703      703           
  Lines      208432   208498   +66     
  Branches    40200    40203    +3     
=======================================
+ Hits       184502   184564   +62     
- Misses      15957    15962    +5     
+ Partials     7973     7972    -1     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/resolve.js	96.20% <100.00%> (+<0.01%)	⬆️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[CraigMacomber]

Out of interest, you never linked the exact test case – is it genuinely hammering invalid import attempts?

The specific test being run is didn't matter (there are no imports inside the test). I haven't tried lots of other workloads, but I suspect that loading any file which transitively imports lots of files with either relative path (like "./util/index.js") or npm package imports (like "@fluidframework/core-utils/internal") throws exceptions for every import even when they are all valid. Our codebase never uses URLs in imports: using URLs in imports seems pretty uncommon in large JS codebases that I have seen (at least when debugging local builds).

In this particular case my workflow as to run mocha in https://github.com/microsoft/FluidFramework/tree/main/packages/dds/tree which will import all the files under https://github.com/microsoft/FluidFramework/tree/main/packages/dds/tree/src/test which in turn transitively import likely thousands of files from various packages and you get the big slow down even before the first test runs (I just ran a single test when measuring so I avoided measuring the overhead of running the full test suite as by that point imports are done and this is no longer an issue). My team commonly runs/debugs single tests (we use a VS-Code extension which provides a button for doing this), and all of them exhibit this large delay (which is actually ~77 seconds back in node 20) if debugging with break on caught exceptions, which was so slow I had to figure out what the heck was going on.

Running our tests under CommonJS did not have this issue (this was a regression from when I updated them to use esm), so I suspected it was related to ESM module loading. Thus when I saw tons of exceptions with "esm" in the file path, I assumed that was the issue, and made this fix to check, confirming this was the problem. I haven't done more investigation beyond that.

The slow down is entirely due to VS-Code somehow allow listing these exceptions (which are immediately caught and ignored) and not breaking on them, and that process being slow: if run without their debugger the load time is ~3 seconds which is reasonable.

[Renegade334]

For consistency, the shouldBeTreatedAsRelativeOrAbsolutePath branch should probably use the same pattern as well:

https://github.com/nodejs/node/blob/main/lib/internal/modules/esm/resolve.js#L838-L845

[CraigMacomber]

I'm a bit unclear on how the commit message guidance in https://github.com/nodejs/node/blob/e28656a6172c35d88a472e33894a11f0e9f98eee/doc/contributing/pull-requests.md#commit-message-guidelines applies to doing updates to the PR (documented in https://github.com/nodejs/node/blob/e28656a6172c35d88a472e33894a11f0e9f98eee/doc/contributing/pull-requests.md#step-9-discuss-and-update )

The example update does not include a message. Previously I thought maybe I was suppose to keep the whole change on my branch described by its commit message with a clean history, so I amended to fix an issue, but I know force pushes make review on github harder and it seems discouraged the document linked above, so I didn't do that for the switch to use URLParse.

Let me know if/how I should adjust the history and commit messages on this branch if needed (and maybe add a little more detail to https://github.com/nodejs/node/blob/e28656a6172c35d88a472e33894a11f0e9f98eee/doc/contributing/pull-requests.md#step-9-discuss-and-update about how to phrase commit messages at this point: should they be phrased for an audience that knows they are part of this PR, or will they live past the merge into main (for example if this PR gets rebase merged and not squashed) and need to be phrased such that they make sense in a more global scope?).

While on the subject of the pull-requests.md file, its section on "Opening the pull request" could use some guidance on the description. Should the first line from the commit message guidance be used as the PR title? The rest for the body? Or is that duplicating information in the commit, and we should describe it some other way?

[aduh95]

node/doc/contributing/pull-requests.md

Lines 540 to 547 in aa8c4f3

	### Commit squashing
	In most cases, do not squash commits that you add to your pull request during
	the review process. When the commits in your pull request land, they may be
	squashed into one commit per logical change. Metadata will be added to the
	commit message (including links to the pull request, links to relevant issues,
	and the names of the reviewers). The commit history of your pull request,
	however, will stay intact on the pull request page.

TL;DR commit messages for follow up commits do not matter, they (the messages, not the diffs) will be discarded upon landing. PR title and description hardly matter for the automation, so you can put whatever really, it's between you and the reviewers.

[CraigMacomber]

Is there more I should do here? That CI error seems unrelate to my change (it's a timeout on Mac, and there is no Mac specific code here and I don't have a Mac to test on).

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 06bf489