Skip to content

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 28, 2021

Import assertion support has landed behind a flag in V8 for quite some time, this PR enables it by default in Node.js. This PR allows folks to write JS code that uses import assertions syntax. Only type: “json” is supported for now, although JSON modules support in Node.js is still experimental and behind --experimental-json-modules CLI flag.
This PR also disables importing of JSON modules without using an assertion, by popular demand. Support for those can be added using loader hooks.

Refs: #37375 (comment)
Alternative to #39921

/cc @nodejs/modules @nodejs/loaders

@aduh95 aduh95 added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. dont-land-on-v12.x labels Sep 28, 2021
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 28, 2021
@aduh95 aduh95 force-pushed the unflag-import-assertion-and-remove-assertionless-json-imports branch from 11f7f0e to 9ee9fdc Compare September 28, 2021 22:39
@ljharb
Copy link
Member

ljharb commented Sep 28, 2021

Is there no way we could add a separate flag that allows assertionless JSON import?

@GeoffreyBooth
Copy link
Member

@aduh95 Thank you for doing this, I really appreciate it. I don’t have any objections with your intent of this PR, so I’m happy for it to land once we can work through some technical discussions. I appreciate your patience.

In the meantime, can we close the other versions of this so that there’s no confusion? #39921 and #40210, are there any others? So that in the end it’s just the PR that adds support for assertions (this PR) and the PR that unflags JSON modules (#37375).

Also #40249 feels pretty interdependent on this PR; I think it should be fine, but it might change depending on discussions in this one, so could we maybe close that one and land that change as part of this?

Re “don’t land on v14.x,” is that because the version of V8 in 14.x doesn’t support unflagged assert?

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 29, 2021

Re “don’t land on v14.x,” is that because the version of V8 in 14.x doesn’t support unflagged assert?

Correct, it doesn't support assert syntax at all (that was added in V8 9.1 IIRC, Node.js v14.x uses V8 8.4).

@GeoffreyBooth
Copy link
Member

@aduh95 and I discussed this. Here’s what I think are the to-do items to land this PR:

  • See if we can keep the import assertions out of the module cache key. Currently the resolve hook’s only required return value is the resolved module URL, which becomes the key; and then load returns the source, which is the module cache value. This gets significantly more complicated if we have a combined key of URL plus assertion, so I’d like to see if we can avoid that if at all possible.

  • Validating the assertion should be isolated in its own function, so that we can determine where it should be called from.

  • The validation logic should probably be called from within defaultLoad, so that it can be overridden/ignored by user loaders (we should discuss this with the larger loaders group and especially @JakobJingleheimer and @bmeck at next week’s loaders meeting).

  • The assertions need to be passed into the resolve hook on its context object, and probably also into the load hook on its context object. We could also allow user resolve hooks to optionally return an assertions object, which would then replace (?) the load hook’s context.assertions. This is something else we should discuss in the next loaders meeting.

@GeoffreyBooth GeoffreyBooth force-pushed the unflag-import-assertion-and-remove-assertionless-json-imports branch 2 times, most recently from 8b18aaf to f889864 Compare October 10, 2021 23:26
@GeoffreyBooth GeoffreyBooth marked this pull request as draft October 11, 2021 15:39
@GeoffreyBooth GeoffreyBooth removed the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Oct 12, 2021
@GeoffreyBooth GeoffreyBooth force-pushed the unflag-import-assertion-and-remove-assertionless-json-imports branch from 17f81db to 13b63c2 Compare October 13, 2021 05:52
@GeoffreyBooth GeoffreyBooth force-pushed the unflag-import-assertion-and-remove-assertionless-json-imports branch from 13b63c2 to 10ba639 Compare October 14, 2021 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.