Non-unique V8 scriptUrl when using dynamic import in CJS

[demurgos]

• Version: 10.12.0
• Platform: Linux 64 bit
• Subsystem: Modules

This bug is a regression in 10.12.0, it worked in 10.11.0.
I am currently working on Node tooling and code coverage using the V8 profiler. When requesting profiling data, V8 returns an object per script. This object has a url value (the value passed to V8 when creating the module).
To support coverage, the URL should be unique: for each file, there should be unique profiling data.

The latest Node version breaks this unicity when dealing with dynamic imports in CJS.

If I have the following main.js (commonJS) file:

import('./hello-world')
  .then(helloWorld => helloWorld.helloWorld())

And execute it (with --experimental-modules), V8 receives the following modules with the same URL:

// scriptId: 76
// url: file:///data/projects/web/istanbulize/src/test/fixtures/esm-hello-world/main.js
  import {
    executor,
    $default
  } from "";
  export {
    $default as default
  }
  if (typeof executor === "function") {
    // add await to this later if top level await comes along
    executor()
  }

// scriptId: 77
// url: file:///data/projects/web/istanbulize/src/test/fixtures/esm-hello-world/main.js
(function (exports, require, module, __filename, __dirname) { import('./hello-world')
  .then(helloWorld => helloWorld.helloWorld())

});

The first module should not use this url since it does not really correspond to this file but is an internal module to bridge between CJS and ESM. When importing CJS modules from ESM, they get the cjs-facade:// protocol instead of file://. This should be applied here too to fix the ambiguity.

Older versions of Node (10.11.0 and before) used the system path instead of the file URL for the CJS module. You had file:///main.js and /main.js so it was possible to detect this case and ignore the file URL because it corresponds to the wrapper. Now that file URLs are used everywhere (a good thing), it is no longer possible to distinguish the internal wrapper from the real file unless you look at the source code.

[demurgos]

The issue is here:

node/lib/internal/modules/esm/create_dynamic_module.js

Line 51 in 3e3ce22

const module = new ModuleWrap(reexports, `${url}`);

It should not use ${url} but something like reexports:${url}

[devsnek]

@demurgos can you open this issue on https://github.com/nodejs/ecmascript-modules

[devsnek]

also... the cjs module itself should not be using a url at all. that might be a bug.

[bcoe]

@TimothyGu was their work in progress to offer an alternative approach to wrapping a script with context, rather than explicitly adding a header and footer to the script being executed?

This would eliminate the need to differentiate between ESM and CJS files when calculating coverage. If you have an API in the works I can reference, I'd happily do some of the integration work in Node.

[devsnek]

@bcoe there's #21573 to change how we wrap cjs which will get rid of the header/footer as far as coverage is concerned.

[SMotaal]

@devsnek I just realized ecmascript-modules does not provide the issues tab. So triage and pathways for issues relating to ES module features is something we should try to disambiguate 😄. The biggest issue is to be able to figure when issues should be addressed in --experimental-modules or future ESM.

[devsnek]

@MylesBorins can we get the issues tab enabled?

[MylesBorins]

@devsnek as this is a bug specifically in the CJS interop implementation that no longer exists in ecmascript-modules it doesn't really make sense to me that we punt it over there. We removed the issues tab specifically as discussion was meant to happen in the modules repo. If you still think we should bring back issues to ecmascript-modules please open an issue in nodejs/modules and we can discuss as a team (sorry for the run around)

What I find particularly odd here is the regression happening in 10.12.0... AFAICT the only change was
removing internal bindings... e7f710c#diff-dc52871f4892835e4b6b339ec0f4b181

As such I think this is something we should be exploring in core not in the fork

[devsnek]

@MylesBorins i was under the impression we weren't maintaining the node core implementation. if this is fixed in ecmascript-modules i would assume this issue can be closed.

as for the issues tab, i would expect issues/bugs with the implementation itself to be put on that repo, not the modules repo. that may just be me though.