Review requested:

• @nodejs/loaders

Codecov Report

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

@@            Coverage Diff             @@
##             main   #62462      +/-   ##
==========================================
- Coverage   89.70%   89.69%   -0.01%     
==========================================
  Files         678      678              
  Lines      207195   207190       -5     
  Branches    39730    39734       +4     
==========================================
- Hits       185862   185839      -23     
- Misses      13438    13439       +1     
- Partials     7895     7912      +17     

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.

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.

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.

Sounds good, can you make a suggestion?