module: clarify cjs global-like error on ModuleJobSync

[Ceres6]

This PR copies the error message used for cjs global-like import errors from ModuleJob to ModuleJobSync to cover more cases.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[Ceres6]

cc @JakobJingleheimer @jsumners

[Ceres6]

There is still one case that I think should be covered by this message highlighted by https://github.com/nodejs/node/blob/main/test/es-module/test-esm-detect-ambiguous.mjs#L212 where an import cjs.js in a package with type: module has the same behaviour but doesn't trigger the message as the error is caught by the module that is importing it, which could be mjs or cjs

[jsumners]

For my own clarification: why am I tagged on this one?

[Ceres6]

For my own clarification: why am I tagged on this one?

Not sure if I tagged wrong. I saw this repro on the nodejs loaders group https://github.com/jsumners-nr/exports-bug

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.22%. Comparing base (9ec7bed) to head (2b6cb3f).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56491      +/-   ##
==========================================
+ Coverage   89.20%   89.22%   +0.01%     
==========================================
  Files         662      662              
  Lines      191934   191949      +15     
  Branches    36944    36951       +7     
==========================================
+ Hits       171218   171261      +43     
+ Misses      13552    13535      -17     
+ Partials     7164     7153      -11     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/module_job.js	99.28% <100.00%> (+0.02%)	⬆️

... and 25 files with indirect coverage changes

[JakobJingleheimer]

For my own clarification: why am I tagged on this one?

@jsumners you sort of requested this in slack about a month ago :)

[jsumners]

For my own clarification: why am I tagged on this one?

Not sure if I tagged wrong. I saw this repro on the nodejs loaders group https://github.com/jsumners-nr/exports-bug

Thank you. Please tag @jsumners-nr instead. That is my work account.

[JakobJingleheimer]

I presume you're asking so it will give you a reminder (which it won't do when you tag yourself). @jsumners-nr :)

[jsumners]

I participate in the loaders group through my work persona. I will review when I am back at work tomorrow. Technically, I wouldn't be tagging myself since it is separate accounts. I'm really just trying to make it clear which account to use for these things.

[Ceres6]

Can we move this forward? @JakobJingleheimer
Did you have time to check it? @jsumners-nr

[jsumners-nr]

Looks good to me.

[jsumners-nr]

Can we move this forward? @JakobJingleheimer Did you have time to check it? @jsumners-nr

I was out all last week due to illness.

[nodejs-github-bot]

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

[joyeecheung]

You can remove the TODO in L395 now because that's talking about...this :)

The other TODO relies on the V8 13.0 upgrade which is blocked on...the macOS upgrade in the CI, sigh.

[Ceres6]

It seems the two failing test are because --experimental-permission is no longer experimental and test-runner-module-mocking.js doesn't exist (maybe is test-runner-mocking.js?) I tried rebasing locally but didn't work so not sure what to do

[joyeecheung]

Those two came from security release hiccups and should be fixed by 01dd7c4 - not sure about whether the GitHub actions are able to rebase. Can you rebase locally and force push here?

[Ceres6]

Done