esm: non file baseurls and fix cache key collision

[bmeck]

fixes: #38714

I'm not actually sure if this qualifies as a breaking change since it imports from the same place just the stack trace for the modules made from stdin/execArgv are given new locations (no longer pointing to non-existant files).

This removes the cache key collision to avoid some problems with policies as well.

CC: @nodejs/modules

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[aduh95]

Nice, it was on my to do list to implement something like that, glad you beat me to it :) marking this semver-major might be prudent given it seems to break some tests…

[JakobJingleheimer]

Thanks for the fix! Generally 👍 but (I?) need a code comment to understand one of your changes.

[aduh95]

$ out/Release/node --input-type=module -e 'import "./nope"'
node:internal/errors:465
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '…/nope' imported from …/
    at new NodeError (node:internal/errors:372:5)
    at finalizeResolution (node:internal/modules/esm/resolve:413:11)
    at moduleResolve (node:internal/modules/esm/resolve:972:10)
    at defaultResolve (node:internal/modules/esm/resolve:1181:11)
    at ESMLoader.resolve (node:internal/modules/esm/loader:592:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:306:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:80:40)
    at link (node:internal/modules/esm/module_job:78:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v18.0.0-pre
$ node --input-type=module -e 'import "./nope"'       
node:internal/errors:465
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '…/nope' imported from …/[eval1]
    at new NodeError (node:internal/errors:372:5)
    at finalizeResolution (node:internal/modules/esm/resolve:404:11)
    at moduleResolve (node:internal/modules/esm/resolve:963:10)
    at defaultResolve (node:internal/modules/esm/resolve:1172:11)
    at ESMLoader.resolve (node:internal/modules/esm/loader:580:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:294:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:80:40)
    at link (node:internal/modules/esm/module_job:78:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v17.7.2

Maybe it's too tricky to fix to be worth it, but it'd be nice if the error message also used the "fake" node:execArgv (and node:stdin) URL.

[aduh95]

CJS is using [stdin] instead of node:stdin for this, should we chose node:[stdin] instead? I don't feel strongly about this at all, but if there are no drawbacks maybe we should aim for consistency.

On another note, I've noticed that this PR makes the following work:

$ echo 'export {test} from "node:stdin"' > test.mjs
$ out/Release/node --input-type=module <<EOF
export let test = 4;
import { test as test2 } from './test.mjs';
console.log({test,test2})
test++;
console.log({test,test2});
EOF
{ test: 4, test2: 4 }
{ test: 5, test2: 5 }

While it doesn't seem to work at all on master:

$ echo 'export {test} from "./[eval1]"' > test.mjs
$ node --input-type=module <<EOF
export let test = 4;
import { test as test2 } from './test.mjs';
console.log({test,test2})
test++;
console.log({test,test2});
EOF
node:internal/errors:465
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '…/[eval1]' imported from …/test.mjs
    at new NodeError (node:internal/errors:372:5)
    at finalizeResolution (node:internal/modules/esm/resolve:404:11)
    at moduleResolve (node:internal/modules/esm/resolve:963:10)
    at defaultResolve (node:internal/modules/esm/resolve:1172:11)
    at ESMLoader.resolve (node:internal/modules/esm/loader:580:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:294:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:80:40)
    at link (node:internal/modules/esm/module_job:78:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v17.7.2

[bmeck]

wrapping both in [] seems good for consistency, then we could also use [eval] like CJS.

I'm fine removing the relative importing for these modules as long as it gives a little more consistency with CJS where you can use require() to get relative files from [stdin].

[bmeck]

Maybe it's too tricky to fix to be worth it, but it'd be nice if the error message also used the "fake" node:execArgv (and node:stdin) URL.

It isn't a hard fix but it might be confusing, maybe we could workshop an error message that you like that includes both?

"Error [ERR_MODULE_NOT_FOUND]: Cannot find module '…/nope' imported from node:[eval]" which resolves relative to "…/"

One of the big things I want is to keep some level of actionability rather than just saying which module did the importing.

[aduh95]

Maybe it's too tricky to fix to be worth it, but it'd be nice if the error message also used the "fake" node:execArgv (and node:stdin) URL.

It isn't a hard fix but it might be confusing, maybe we could workshop an error message that you like that includes both?

"Error [ERR_MODULE_NOT_FOUND]: Cannot find module '…/nope' imported from node:[eval]" which resolves relative to "…/"

One of the big things I want is to keep some level of actionability rather than just saying which module did the importing.

Would you add the which resolves from part for all ERR_MODULE_NOT_FOUND errors or just the [eval]+[stdin]? The error message is clear enough to me without it, but no objection on my side to add more info if it's helpful to others 👍

[bmeck]

Likely just emit the extra info when cache key doesn't match baseURL

…

On Mon, Mar 21, 2022, 5:51 PM Antoine du Hamel ***@***.***> wrote: Maybe it's too tricky to fix to be worth it, but it'd be nice if the error message also used the "fake" node:execArgv (and node:stdin) URL. It isn't a hard fix but it might be confusing, maybe we could workshop an error message that you like that includes both? "Error [ERR_MODULE_NOT_FOUND]: Cannot find module '…/nope' imported from node:[eval]" which resolves relative to "…/" One of the big things I want is to keep some level of actionability rather than just saying which module did the importing. Would you add the which resolves from part for all ERR_MODULE_NOT_FOUND errors or just the [eval]+[stdin]? The error message is clear enough to me without it, but no objection on my side to add more info if it's helpful to others 👍 — Reply to this email directly, view it on GitHub <#42392 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJIZK2T4HZFHYV5AZB2LVBD4PVANCNFSM5RCUZ3EA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[aduh95]

$ echo 'export * from "node:stdin"' | out/Release/node --input-type=module # works fine
$ echo 'export * from "node:stdin"' | out/Release/node --input-type=module -e 'import "node:stdin"'
node:internal/errors:465
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:stdin
    at new NodeError (node:internal/errors:372:5)
    at ESMLoader.builtinStrategy (node:internal/modules/esm/translators:260:11)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:349:14) {
  code: 'ERR_UNKNOWN_BUILTIN_MODULE'
}

Node.js v18.0.0-pre

Would it make sense to special-case node:[stdin] so it doesn't throw an error that says it's unknown? I guess we could either bring back ERR_UNKNOWN_STDIN_TYPE, or create a new error code – or even leave it as is, it probably doesn't need to block the PR from landing.

[Mifrill]

why we are mutating baseURLs on the call getter (getBaseURL)?