[v18.x backport] esm: use import attributes instead of import assertions

[nicolo-ribaudo]

Backport of https://github.com/nodejs/node/pull/50140, closes https://github.com/nodejs/node/issues/51118

That PR has already been fully backported to v20 (https://github.com/nodejs/node/pull/50183), but it was only partially backported to v18 (https://github.com/nodejs/node/pull/50329).

This PR backports syntax support from V8, that was only backported to v20. With this backport all maintained LTS versions support the new syntax, making it possible for libraries to actually migrate.

cc @aduh95

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/loaders
• [ ] @nodejs/startup
• [ ] @nodejs/v8-update

[targos]

We may also need https://github.com/v8/v8/commit/d90d4533b05301e2be813a5f90223f4c6c1bf63d

[nicolo-ribaudo]

Node.js 18 is not affected by the bug fixed by that V8 commit. Running

import("./package.json", { assert: { 0: 'value', type: 'json' } }).then(console.log, console.error)

correctly reports a TypeError

TypeError: Import assertion value must be a string
    at REPL1:1:1
    at Script.runInThisContext (node:vm:122:12)
    at REPLServer.defaultEval (node:repl:594:29)
    at bound (node:domain:433:15)
    at REPLServer.runBound [as eval] (node:domain:444:12)
    at REPLServer.onLine (node:repl:924:10)
    at REPLServer.emit (node:events:529:35)
    at REPLServer.emit (node:domain:489:12)
    at [_onLine] [as _onLine] (node:internal/readline/interface:423:12)
    at [_line] [as _line] (node:internal/readline/interface:894:18)

OTOH, that code makes Node.js 20 crash (so I could backport it to Node.js 20, but as a separate PR).

[nicolo-ribaudo]

Regarding the CI failure, I'm not sure how it's related to these changes (since this PR does not touch the vm module except for docs, and that test doesn't use assertions/attributes). Also, I cannot reproduce it by running ./out/Release/node --experimental-vm-modules test/parallel/test-vm-timeout-escape-promise-module.js locally.

[targos]

I already backported it to v20.x. What's missing is https://github.com/nodejs/node/pull/50703.

[targos]

The CI failure happens with the ASan build (./configure --enable-asan)

[nicolo-ribaudo]

I added https://github.com/nodejs/node/pull/50703 since the bug affects v18, I will open a PR backporting it to v20 if needed.

The CI failure happens with the ASan build (./configure --enable-asan)

Thanks! I'm debugging this locally (in the meanwhile, could you approve the GH workflows since this is my first PR?)

[targos]

I'm ready to push it to v20.x-staging with https://github.com/nodejs/node/pull/50181. See https://github.com/nodejs/node/pull/51124#issuecomment-1854138679

[nicolo-ribaudo]

@targos In case #50181 ends up not being backported (I assume it's because changing error codes in LTSs is potentially dangerous?), you can cherry-pick src: iterate on import attributes array correctly from this PR (I updated the test to use the old error code)

[nicolo-ribaudo]

CI is green now but I didn't do anything explicitly to fix it 🤔 Is it possible that one of the VM tests is flaky with ASan?

[nicolo-ribaudo]

Hey, is there any interest in this? :)

[aduh95]

v18.x is in maintenance, meaning there's no more scheduled releases, releases only happen for security patches and when someone from the Releasers team volunteers to make a new release. What lands on the LTS release line is also at the discretion of whoever makes the release, so until someone starts working on a new release, backport PRs stay open. For reference, this is defined at https://github.com/nodejs/release?tab=readme-ov-file#release-phases.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[richardlau]

Landed in 68fd7516e1cc...96514a8b9fdb.

[nicolo-ribaudo]

Thank you!