esm: non file baseurls and fix cache key collision #42392

bmeck · 2022-03-18T18:57:14Z

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 · 2022-03-18T18:57:18Z

Review requested:

@nodejs/modules

aduh95 · 2022-03-18T23:35:43Z

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.

lib/internal/modules/esm/loader.js

lib/internal/process/execution.js

Co-authored-by: Jacob Smith <[email protected]>

aduh95 · 2022-03-21T20:42:11Z

$ 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 · 2022-03-21T20:55:27Z

lib/internal/main/eval_stdin.js

@@ -24,7 +24,7 @@ readStdin((code) => {

  const print = getOptionValue('--print');
  if (getOptionValue('--input-type') === 'module')
-    evalModule(code, print);
+    evalModule(code, print, 'node:stdin');


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

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 · 2022-03-21T21:22:26Z

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 · 2022-03-21T22:51:26Z

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 · 2022-03-21T23:03:32Z

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 · 2022-03-21T23:05:21Z

$ 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 · 2023-12-13T16:09:22Z

lib/internal/modules/esm/loader.js

        throw new ERR_INTERNAL_ASSERTION(`Base url for module ${url} not loaded.`);
      }
+      this.baseURLs.set(url, baseURL);


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

bmeck added 2 commits March 18, 2022 13:26

esm: fix baseURL for non-files

0cdd803

fixup: make import.meta properly init

cfdd468

bmeck requested review from guybedford and aduh95 March 18, 2022 18:57

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 18, 2022

bmeck added cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation. and removed lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 18, 2022

bmeck added 3 commits March 18, 2022 14:03

fixup: oopsie

ff4b7c0

fixup: oopsie

d80ab5b

fixup: lint

1513e3c

JakobJingleheimer reviewed Mar 20, 2022

View reviewed changes

lib/internal/modules/esm/loader.js Show resolved Hide resolved

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

lib/internal/process/execution.js Outdated Show resolved Hide resolved

bmeck and others added 3 commits March 21, 2022 09:56

fixup: Update lib/internal/modules/esm/loader.js

b6b2a91

Co-authored-by: Jacob Smith <[email protected]>

fixup: review nits

3c518c2

fixup: lint

933139e

aduh95 reviewed Mar 21, 2022

View reviewed changes

aduh95 mentioned this pull request Apr 3, 2022

esm: use [stdin] as module URL when reading stdin #41946

Closed

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Mifrill reviewed Dec 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: non file baseurls and fix cache key collision #42392

esm: non file baseurls and fix cache key collision #42392

bmeck commented Mar 18, 2022 •

edited

Loading

nodejs-github-bot commented Mar 18, 2022

aduh95 commented Mar 18, 2022

JakobJingleheimer left a comment

aduh95 commented Mar 21, 2022 •

edited

Loading

aduh95 Mar 21, 2022

bmeck Mar 21, 2022

bmeck commented Mar 21, 2022

aduh95 commented Mar 21, 2022

bmeck commented Mar 21, 2022 via email

aduh95 commented Mar 21, 2022

Mifrill Dec 13, 2023 •

edited

Loading

esm: non file baseurls and fix cache key collision #42392

Are you sure you want to change the base?

esm: non file baseurls and fix cache key collision #42392

Conversation

bmeck commented Mar 18, 2022 • edited Loading

nodejs-github-bot commented Mar 18, 2022

aduh95 commented Mar 18, 2022

JakobJingleheimer left a comment

Choose a reason for hiding this comment

aduh95 commented Mar 21, 2022 • edited Loading

aduh95 Mar 21, 2022

Choose a reason for hiding this comment

bmeck Mar 21, 2022

Choose a reason for hiding this comment

bmeck commented Mar 21, 2022

aduh95 commented Mar 21, 2022

bmeck commented Mar 21, 2022 via email

aduh95 commented Mar 21, 2022

Mifrill Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

bmeck commented Mar 18, 2022 •

edited

Loading

aduh95 commented Mar 21, 2022 •

edited

Loading

Mifrill Dec 13, 2023 •

edited

Loading