Module: Import assertions improvements by GeoffreyBooth · Pull Request #40785 · nodejs/node · GitHub

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 https://github.com/nodejs/node/discussions/40790#discussioncomment-1673795, 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

Implementation LGTM, discussion is ongoing though

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.

RSLGTM

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.

Blocking as per my comments in https://github.com/nodejs/node/discussions/40790#discussioncomment-1647538. 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.

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

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

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

Comment thread doc/api/esm.md Outdated

Comment thread test/es-module/test-esm-assertionless-javascript-import.js Outdated

Comment thread test/es-module/test-esm-assertionless-javascript-import.js Outdated

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

Comment thread test/es-module/test-esm-assertionless-javascript-import.js Outdated

Comment thread doc/api/esm.md Outdated

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

Module: Import assertions improvements by GeoffreyBooth · Pull Request #40785 · nodejs/node · GitHub

Uh oh!

Uh oh!

Module: Import assertions improvements - #40785

Module: Import assertions improvements#40785GeoffreyBooth wants to merge 2 commits intonodejs:masternodejs/node:masterfrom GeoffreyBooth:more-import-assertion-typesGeoffreyBooth/node:more-import-assertion-typesCopy head branch name to clipboard

Conversation

GeoffreyBooth commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

bmeck commented Nov 15, 2021

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

This comment has been minimized.

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

nodejs-github-bot commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Module: Import assertions improvements#40785
GeoffreyBooth wants to merge 2 commits into
nodejs:masterfrom
GeoffreyBooth:more-import-assertion-types

GeoffreyBooth commented Nov 11, 2021 •
edited
Loading