Bug: import() doesn’t work in string input

[GeoffreyBooth]

Using nodejs/node@ceb80f4 on Mac:

// test.mjs
(async function() { await import("./file.mjs"); })();

// file.mjs
import { version } from "process"; console.log(version);

When working with files, the output is as expected:

$ ./node --experimental-modules ./test.mjs
(node:99113) ExperimentalWarning: The ESM module loader is experimental.
v12.0.0-pre

But when the same input is passed in as a string, we get an unexpected error:

$ ./node --experimental-modules --entry-type=module < ./test.mjs
(node:99398) ExperimentalWarning: The ESM module loader is experimental.
(node:99398) UnhandledPromiseRejectionWarning: TypeError [ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING]: A dynamic import callback was not specified.
    at exports.importModuleDynamicallyCallback (internal/process/esm_loader.js:33:9)
    at eval:1:2:21
    at eval:1:2:51
    at ModuleJob.run (internal/modules/esm/module_job.js:106:37)
    at processTicksAndRejections (internal/process/task_queues.js:88:5)
    at process.runNextTicks [as _tickCallback] (internal/process/task_queues.js:58:3)
    at evalModule (internal/process/execution.js:53:11)
    at internal/main/eval_stdin.js:21:5
    at ReadStream.<anonymous> (internal/process/execution.js:192:5)
    at ReadStream.emit (events.js:201:15)
(node:99398) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:99398) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

The same error appears for the following:

$ ./node --experimental-modules --entry-type=module \
  --eval '(async function() { await import("./file.mjs"); })();'

$ ./node --experimental-modules --entry-type=module \
  --print '(async function() { await import("./file.mjs"); })();'

related: nodejs/node#19570

[devsnek]

this is pending a redesign of ModuleWrap and Contextify registration. in the short term we can set the import dynamically callbacks for them but they leak like hell.

[GeoffreyBooth]

I’m working on a PR related to --input-type and I’m having to disable some tests because of this. Is there anything I can do to fix this in the short term? Disable the error?

[devsnek]

add an importModuleDynamically callback to https://github.com/nodejs/node/blob/ceb80f415798818a059051537132bba691c68db7/lib/repl.js#L356

@guybedford at this point do you think its worth trying to convince v8 to change host_defined_options to a void* or Local<Value>

[GeoffreyBooth]

What should the callback be? Just a noop function?

And are you sure that’s the place? The REPL isn’t in the stack trace.

[devsnek]

@GeoffreyBooth oh sorry not just the repl, all the places that use contextify scripts or module wraps need to have the callback, and i think the callback just needs to return ESMLoader.import(specifier, <relevant referrer>).

for module wraps you need to do this (https://github.com/nodejs/node/blob/ceb80f415798818a059051537132bba691c68db7/lib/internal/modules/esm/translators.js#L48)

[GeoffreyBooth]

So should I submit a PR against core that does that? Is there any reason not to?

[devsnek]

@GeoffreyBooth it should be mostly fine. the callbacks and some other associated data are leaked in some cases, which is probably only noticeable with a repl where you've got a large number of short lived scriprts.

[guybedford]

@devsnek I do think we should try move forward with the v8 API upgrading host_defined_options to Local. That form seems important to remain GC compatible I think so we can get garbage collection working out. Previously I was hoping we could find a way to fix the root cause of this, but if we know for sure we can make progress with v8 on the above, then it could make sense to just make the references leaky persistent in the mean time too - but the caveat on this is being sure we can move forward with the given fix architecture.

[devsnek]

@guybedford i'm working on a patch for v8

[MylesBorins]

Will the v8 patch be ABI breaking or be able to be backported?

[devsnek]

@MylesBorins currently not abi compatible: https://chromium-review.googlesource.com/c/v8/v8/+/1553008

[devsnek]

This was fixed a long while ago.