esm: add ERR_REQUIRE_ESM_RACE_CONDITION

[aduh95]

I don't think we have any way to support it, so we shouldn't be throwing ERR_INTERNAL_ASSERTION that requests the user to open an issue here.

Closes: #62432

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[mcollina]

lgtm

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.70%. Comparing base (53bcd11) to head (a7c8049).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/module_job.js	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #62462     +/-   ##
=========================================
  Coverage   89.70%   89.70%             
=========================================
  Files         678      692     +14     
  Lines      207195   213981   +6786     
  Branches    39730    41057   +1327     
=========================================
+ Hits       185862   191952   +6090     
- Misses      13438    14086    +648     
- Partials     7895     7943     +48     

Files with missing lines	Coverage Δ
lib/internal/errors.js	97.63% <100.00%> (+0.01%)	⬆️
lib/internal/modules/esm/loader.js	99.68% <100.00%> (+0.91%)	⬆️
lib/internal/modules/esm/module_job.js	96.39% <33.33%> (-0.17%)	⬇️

... and 78 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.

[joyeecheung]

I think a dedicated error is indeed more user friendly for now, though we may need some text suggesting this can go away in future versions and users should avoid rely on being able to catch one/prepare that it may "just work" in a future version - there are a couple of preconditions that can make it disappear, e.g. async loading path going away after module.register go away, and the landing of import defer in V8/ECMA262 will help supporting the cyclic references, and it would be counter productive (may not even be practical) if "making this error go away" must be semver-major.

[joyeecheung]

This should be moved to known-issues if we want to formulate this as a "FIXME" instead of "working as intended"

[aduh95]

Moving it to known_issues I could not check the error code, not sure it's desirable

[aduh95]

there are a couple of preconditions that can make it disappear, e.g. async loading path going away after module.register go away, and the landing of import defer in V8/ECMA262 will help supporting the cyclic references, and it would be counter productive (may not even be practical) if "making this error go away" must be semver-major.

Unless I'm missing something, both removal of module.register and V8 upgrade would not happen in a semver-minor, so it should be fine if it was semver-major, no? In any case, unless it's somehow breaking the ecosystem, adding a throw would be semver-major, removing one would not be.

[joyeecheung]

both removal of module.register and V8 upgrade would not happen in a semver-minor, so it should be fine if it was semver-major, no?

They are preconditions but would not in themselves fix the issue automatically, so they could land in semver major but the actual disappearance of the error need to be implemented some time after. If we confuse people into thinking this is an always catchable error that makes it harder to land fixes in a minor/patch.

adding a throw would be semver-major, removing one would not be.

I think this is something we would agree but unfortunately not very clear to users (e.g. there were some users arguing disappearance of ERR_REQUIRE_ESM should be semver major because some libraries rely on the appearance of it, while some library authors arguing it should be semver-minor, so it was at least something people consider debatable instead of well-agreed). Since this error is just a temporary limitation instead of an intentional design, a heads up in the doc would hopefully steer people away from counting on it at all and into using more robust solutions for whatever they are relying on it for, and there generally seem no harm in emphasising it a bit in the docs.

[aduh95]

Sounds good, can you make a suggestion?