Module: Import assertions improvements #40785

GeoffreyBooth · 2021-11-11T05:02:37Z

Please use #40790 for “product” or “feature” discussion, so that the discussion on this PR thread can stay focused on technical feedback.

This PR implements #40790 (comment), making the following changes to Node’s support for import assertions:

An experimental warning is now printed for all imports with explicitly declared assertion types (currently only JSON).
In the module map, the string 'javascript' is used instead of a symbol to represent the “default” or implied type, to align with the HTML spec.
To align with Chrome and with Prevent modules from being imported with a type: 'javascript' assertion whatwg/html#7350, assert { type: 'javascript' } throws an error. (This was the behavior before, but now it’s made explicit since we use 'javascript' internally.)

Please see #40790 for the motivation behind these changes, and please use that thread for discussion of the desired functionality. I’d like to limit the discussion on this PR thread to technical concerns. Thanks! cc @nodejs/modules @nodejs/loaders

doc/api/esm.md

test/es-module/test-esm-assertionless-javascript-import.js

bmeck

Implementation LGTM, discussion is ongoing though

bmeck · 2021-11-15T21:31:54Z

Since the discussion seems to be narrowed to around how to deal w/ imports that lack the assertion, and it seems it would move from early error -> working at runtime. I think we should probably land this PR and continue discussion elsewhere unless there are clear concerns about moving those early errors into working at runtime code from this implementation in this PR. I tried to read the discussion again but don't see anything that stands out to me as absolutely concerned with the implementation here.

MylesBorins

RSLGTM

MylesBorins · 2021-11-15T22:17:47Z

doc/api/esm.md

@@ -582,6 +608,9 @@ node --experimental-wasm-modules index.mjs

 would provide the exports interface for the instantiation of `module.wasm`.

+The `assert { type: 'webassembly' }` syntax is mandatory; see


should we link out to the ongoing discussion in HTML spec?

I’m not sure what the best link would be. There’s WebAssembly/esm-integration#42 or tc39/proposal-import-attributes#19 or others. It also feels a little messy to link to a discussion thread as opposed to a decision outcome. But if you have a link in particular you recommend, though, I’m happy to add it.

guybedford

Blocking as per my comments in #40790 (comment). to summarize:

Separating "type": "javascript" and "type": "webassembly" risks conflating a model of security privilege with the execution model, when JS and Wasm currently share execution privileges by virtue of being able to import and execute any function. Making this distinction I believe would result in unnecessary interop frictions on import boundaries.
Whether "type": "webassembly" will be optional, required or even supported at all is entirely unclear and should be determined based on discussion with those in the browser and Web Assembly communities through the Web Assembly ESM Integration effort, not by Node.js jumping ahead with implementation.

I'm in contact with @GeoffreyBooth with regards to next steps on this topic.

aduh95

LGTM with one nit and assuming Guy concern can be resolved.

test/es-module/test-esm-assertionless-javascript-import.js

doc/api/esm.md

nodejs-github-bot · 2021-11-24T04:33:09Z

Commit Queue failed

- Loading data for nodejs/node/pull/40785
✔  Done loading data for nodejs/node/pull/40785
----------------------------------- PR info ------------------------------------
Title      Module: Import assertions improvements (#40785)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     GeoffreyBooth:more-import-assertion-types -> nodejs:master
Labels     module, esm, author ready, loaders
Commits    2
 - doc: fix spelling of 'WebAssembly'
 - module: import assertions improvements
Committers 1
 - Geoffrey Booth 
PR-URL: https://github.com/nodejs/node/pull/40785
Reviewed-By: James M Snell 
Reviewed-By: Bradley Farias 
Reviewed-By: Myles Borins 
Reviewed-By: Guy Bedford 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40785
Reviewed-By: James M Snell 
Reviewed-By: Bradley Farias 
Reviewed-By: Myles Borins 
Reviewed-By: Guy Bedford 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc: fix spelling of 'WebAssembly'
   ⚠  - module: import assertions improvements
   ℹ  This PR was created on Thu, 11 Nov 2021 05:02:37 GMT
   ✔  Approvals: 5
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/40785#pullrequestreview-805386076
   ✔  - Bradley Farias (@bmeck): https://github.com/nodejs/node/pull/40785#pullrequestreview-811832395
   ✔  - Myles Borins (@MylesBorins) (TSC): https://github.com/nodejs/node/pull/40785#pullrequestreview-806600800
   ✔  - Guy Bedford (@guybedford): https://github.com/nodejs/node/pull/40785#pullrequestreview-811839664
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/40785#pullrequestreview-807849260
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2021-11-24T04:02:39Z: https://ci.nodejs.org/job/node-test-pull-request/41066/
- Querying data for job/node-test-pull-request/41066/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1497846338

nodejs-github-bot · 2021-11-24T15:50:08Z

Landed in 7633c86...6dfbf04

PR-URL: #40785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#40785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #40785 Backport-PR-URL: #41776 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Nov 11, 2021

GeoffreyBooth requested review from MylesBorins and aduh95 November 11, 2021 05:02

GeoffreyBooth self-assigned this Nov 11, 2021

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 11, 2021

GeoffreyBooth mentioned this pull request Nov 11, 2021

Proposal: Support more import assertion types #40766

Closed

jasnell approved these changes Nov 13, 2021

View reviewed changes

aduh95 reviewed Nov 13, 2021

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

test/es-module/test-esm-assertionless-javascript-import.js Outdated Show resolved Hide resolved

test/es-module/test-esm-assertionless-javascript-import.js Outdated Show resolved Hide resolved

GeoffreyBooth force-pushed the more-import-assertion-types branch from 5838a72 to 48c581a Compare November 15, 2021 06:31

bmeck approved these changes Nov 15, 2021

View reviewed changes

MylesBorins approved these changes Nov 15, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

guybedford requested changes Nov 16, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Nov 16, 2021

aduh95 approved these changes Nov 16, 2021

View reviewed changes

test/es-module/test-esm-assertionless-javascript-import.js Outdated Show resolved Hide resolved

doc/api/esm.md Outdated Show resolved Hide resolved

GeoffreyBooth force-pushed the more-import-assertion-types branch from 48c581a to 4ff88dc Compare November 16, 2021 22:32

This comment has been minimized.

Sign in to view

github-actions bot mentioned this pull request Nov 17, 2021

CI Reliability 2021-11-17 nodejs/reliability#120

Open

39 tasks

This comment has been minimized.

Sign in to view

aduh95 approved these changes Nov 24, 2021

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 24, 2021

nodejs-github-bot closed this Nov 24, 2021

ljharb deleted the more-import-assertion-types branch November 24, 2021 15:52

This was referenced Nov 25, 2021

CI Reliability 2021-11-25 nodejs/reliability#128

Open

CI Reliability 2021-11-26 nodejs/reliability#129

Open

targos mentioned this pull request Nov 26, 2021

v17.2.0 release proposal #40983

Merged

This was referenced Nov 27, 2021

CI Reliability 2021-11-27 nodejs/reliability#130

Open

CI Reliability 2021-11-28 nodejs/reliability#131

Open

aduh95 mentioned this pull request Jan 30, 2022

[v16.x Backport] Import assertions related PRs #41776

Closed

aduh95 added the backport-open-v16.x label Jan 30, 2022

danielleadams added backported-to-v16.x and removed backport-open-v16.x labels Feb 1, 2022

danielleadams mentioned this pull request Feb 1, 2022

v16.14.0 proposal #41804

Merged

bioerrorlog mentioned this pull request May 25, 2022

Add error handling to invalid args Japan-DfinityInfoHub/generative-art-nft#36

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module: Import assertions improvements #40785

Module: Import assertions improvements #40785

GeoffreyBooth commented Nov 11, 2021 •

edited

Loading

bmeck left a comment

bmeck commented Nov 15, 2021

MylesBorins left a comment

MylesBorins Nov 15, 2021

GeoffreyBooth Nov 15, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

guybedford left a comment

This comment has been minimized.

aduh95 left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Nov 24, 2021

nodejs-github-bot commented Nov 24, 2021

		@@ -582,6 +608,9 @@ node --experimental-wasm-modules index.mjs

		would provide the exports interface for the instantiation of `module.wasm`.

		The `assert { type: 'webassembly' }` syntax is mandatory; see

Module: Import assertions improvements #40785

Module: Import assertions improvements #40785

Conversation

GeoffreyBooth commented Nov 11, 2021 • edited Loading

bmeck left a comment

Choose a reason for hiding this comment

bmeck commented Nov 15, 2021

MylesBorins left a comment

Choose a reason for hiding this comment

MylesBorins Nov 15, 2021

Choose a reason for hiding this comment

GeoffreyBooth Nov 15, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

guybedford left a comment

Choose a reason for hiding this comment

This comment has been minimized.

aduh95 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Nov 24, 2021

nodejs-github-bot commented Nov 24, 2021

GeoffreyBooth commented Nov 11, 2021 •

edited

Loading