feat: add support for import assertions #4267

dnalborczyk · 2021-11-06T03:35:32Z

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

List any relevant issue numbers:
#3799

node.js support: json modules import assertions was recently merged into master ~~(not yet released)~~ released with v17.1.0: nodejs/node#40250

webpack support: webpack/webpack#12278 (parsing only)

typescript support: microsoft/TypeScript#40698 (parsing only)

babel support: https://github.com/babel/babel/tree/main/packages/babel-plugin-syntax-import-assertions (parsing only)

Description

there's a couple questions we need to figure out about the differences on what is currently supported, and what and how support should look like going forward:

current json support:

needs a plugin
plugin always exports a default export, and named exports if it's a plain object with properties
all other types are being exported as default export only
~~node.js support behind a flag --experimental-json-modules~~, support behind the flag has been removed in v17.1.0. import assertions are now required, including the same flag. only supports default exports
not supported by browsers (and likely never will be)
this should be not possible, but currently is supported:

import foo from "./foo.json";

console.log(foo);

// foo.json

{
  "__proto__": null,
  "foo": 2
}

bundled to:

var __proto__ = null;
var foo = 2;
const foo$1 = {
	__proto__: __proto__,
	foo: foo
};

console.log(foo$1); // this is entirely different than going thru JSON.parse()

another problem JSON.parse() would avoid. since this example is already using the json plugin, we could properly throw that this is not valid "json":

<html />

(!) Plugin json: Could not parse JSON file
foo.json
[!] Error: Unexpected token (Note that you need @rollup/plugin-json to import JSON files)
foo.json (1:0)
1: <html />
   ^
Error: Unexpected token (Note that you need @rollup/plugin-json to import JSON files)

import assertions/json modules

should? need a plugin?
spec indicates json is always a default export, named exports are no supported
it should also be parsed with JSON.parse(), otherwise throw (hence assertion syntax)
supported by node.js (behind --experimental-json-modules)
supported by browsers

I think the most straight forward and spec compliant way would be to export as default and use JSON.parse() at user runtime on the file content. if not json, throw. in addition retire named exports - in a major version - otherwise it would just add more confusion in an ecosystem with a lot of weirdness, fragmentation and inconsistencies. (javascript in general, but also the cjs, esm, amd, umd, systemjs module nightmare [and transition] we are currently in.)

another possible spec compliant alternative could also be to read the file contents and run JSON.parse(...) at rollup runtime. I think I prefer the former, as it aligns with the non-bundled runtime. on the end of the day it might not matter, other than performance #3799 (comment) and the fact that one would throws at user runtime, and the other would throw at rollup runtime (if we don't have valid json). throwing at user runtime should not be considered much of a problem, since javascript (or any scripting language) is doing the same.

edit:

forgot to mention a scenario where import assertions are being used, but declared as external. the import assertion should remain in the code as it otherwise would fail loading unbundled. (currently it is being removed)

import foo from "./foo.json" assert { type: 'json' }
console.log(foo)

// rollup.config.js
export default {
  external: ["./foo.json"],
  // ...
}

becomes:

import foo from "./foo.json"
console.log(foo)

codecov · 2021-11-06T03:40:36Z

Codecov Report

Merging #4267 (e2ec5b1) into master (66b3139) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4267   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         204      204           
  Lines        7288     7288           
  Branches     2081     2081           
=======================================
  Hits         7171     7171           
  Misses         58       58           
  Partials       59       59

Impacted Files	Coverage Δ
src/Graph.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66b3139...e2ec5b1. Read the comment docs.

lukastaegert · 2021-11-07T06:22:28Z

Not so sure yet.

Aligning with Node is definitely a good thing. But how standardized are those assertions now? Do browsers the same for JSON?
IMO it should still be possible for users to plug their own solution
Argument for using JSON.parse: It is likely faster
Argument against it: There will be no static analysis. And that is REALLY a pity because this is one of Rollup's strengths. Take this example.

Update to clarify: You were talking about using JSON.parse at runtime, right?

In the end, the solution could be to

Add the assertions e.g. as an "assert" option to the resolveId hook so that assertions can be handled differently there.
As the effect of the assertion should be mainly when loading, we need to also forward it to the load hook. That means it also needs to be part of the return value of resolveId and plugins need to make sure they pass it along. Maybe we can also implement some mechanism to "remember" the assertion if the plugin does not pass it along.
Then the load hook should get is as additional parameter (actually as part of an additional parameter object to not break e.g. Vite)
And then we can add a default implementation for handling JSON assertions where we should pick a reasonable default, see my arguments above.

lukastaegert · 2021-11-07T07:48:18Z

The longer I think about this, the more I start to rethink this. To make it clear, I think import assertions will become really important and useful if we manage to get them "right", the question is just what that would be.

If as part of the module graph, the same module is imported once with an assertion and once without it, I assume the runtime will treat them as two separate modules, is that right? On the other hand, all imports of a module with the same assertion will be treated as the same module instance? Should be easy to verify this for json with some experiments. If so, this has some implications:
- Either module "ids" used by Rollup are no longer unique ids but are only unique ids in combination with the assertion type. This could prove VERY problematic for the plugin eco system.
- Or we make the assertion part of the module id in some way, e.g. foo\0assert:json using something like \0assert: as separator. There are a lot of details to be fletched out if we go this way.
Moreover, what happens if the user specify an assertion that is neither built-in (i.e. not json) and not handled by a plugin? How does V8 handle unknown assertions, ignore or throw? I would hope it is the latter as that would help prevent bugs that stem from assertions not being processed correctly or typos by the user. However this will add further complexity to finding a solution that has the least impact on the plugin ecosystem. I.e. ideally we find a solution so that "all plugins work when no assertions are used and most plugins already do the right thing without the need to know about assertions when they are used".

dnalborczyk · 2021-11-09T01:23:53Z

Aligning with Node is definitely a good thing. But how standardized are those assertions now? Do browsers the same for JSON?

the original ecma proposal was split into 2 proposals: import assertions and json modules (which is built on top of import assertions).

browsers, at least chrome, already ship json modules (I believe css modules are on their way). node.js will ship it in v17.1.0 nodejs/node#40758

IMO it should still be possible for users to plug their own solution

Argument for using JSON.parse: It is likely faster

Argument against it: There will be no static analysis. And that is REALLY a pity because this is one of Rollup's strengths. Take this example.

good point. would it be possible to use JSON.parse() as an assert-kind-of-thing, but keep going with the static analysis afterwards? unfortunately that's beyond my knowledge. 😞

Update to clarify: You were talking about using JSON.parse at runtime, right?

I was just throwing out ideas. I think it could be done at user runtime or rollup built time. I have to think it thru again, but I think it should definitely go thru JSON.parse one way or another.

Personally I also strongly believe json should be exported as default only, to lessen confusion. otherwise user code will not run unbundled (in node.js and the browsers), which is being used a lot in development and tests. on the other hand I guess people will eventually figure it out on their own.

dnalborczyk · 2021-11-09T01:47:43Z

The longer I think about this, the more I start to rethink this. To make it clear, I think import assertions will become really important and useful if we manage to get them "right", the question is just what that would be.

the assertion was added to the ecma spec for browsers, as they use the mime-type and not the file extension to get the module type. whereas node.js uses file extensions as well as the package.json type property. with an assertion the developer can make sure that the server does not unexpectedly send a different mime-type and code could potentially be executed: https://github.com/tc39/proposal-import-assertions#motivation

that's why node.js is a bit confusing in that regard, and one could argue the type assertion is not needed.

while this would run in he browser, the same would throw throw in node.js:

import foo from './foo.js' assert { type: 'json' } // if we have valid json in the js file

there was, and probably still is, a big debate about if assertion-less syntax should be allowed in node.js or not. currently you need a flag to use either.

If as part of the module graph, the same module is imported once with an assertion and once without it, I assume the runtime will treat them as two separate modules, is that right?

interestingly this came up in the node.js PR as well, and I'm not quite sure if it has been resolved, nor why this was an issue to begin with. I'll have to read thru the PR comments again.

that said, I'm 99% sure that currently the module instance is the same when loaded with and without an assertion. (keep in mind, this would not be possible in the browser, only in node.js).

On the other hand, all imports of a module with the same assertion will be treated as the same module instance?

yes.

Should be easy to verify this for json with some experiments. If so, this has some implications:

Either module "ids" used by Rollup are no longer unique ids but are only unique ids in combination with the assertion type. This could prove VERY problematic for the plugin eco system.

Or we make the assertion part of the module id in some way, e.g. foo\0assert:json using something like \0assert: as separator. There are a lot of details to be fletched out if we go this way.

see above.

Moreover, what happens if the user specify an assertion that is neither built-in (i.e. not json) and not handled by a plugin? How does V8 handle unknown assertions, ignore or throw?

Not quite sure, that might be up to the host to decide what to do.

currently chrome throws if there's an unknown assert type:

import('./foo.js', { assert: { type: "j" } })
Promise {<rejected>: TypeError: "j" is not a valid module type.
    at <anonymous>:1:1}

I would hope it is the latter as that would help prevent bugs that stem from assertions not being processed correctly or typos by the user. However this will add further complexity to finding a solution that has the least impact on the plugin ecosystem. I.e. ideally we find a solution so that "all plugins work when no assertions are used and most plugins already do the right thing without the need to know about assertions when they are used".

good question. for json, and soon css, plugins should do what the spec recommends, and what engines implement. I think I would not allow other assertion types as well, otherwise you'd deviate from the spec. that way future type assertions can be supported easily without breaking the entire ecosystem.

lukastaegert · 2021-11-09T05:30:12Z

Thanks for looking into this, wow, I did not expect it worked this way. But I could confirm: You can actually add "nonsense assertions" in Chrome safely, and they will just give you the same module instance:

// foo.js
export default {};

// main.js
import a from './foo.js' assert { foo: 'a' };
import b from './foo.js' assert { foo: 'b' };
import c from './foo.js'; 

console.log(a === b); // true
console.log(a === c); // true

I guess as "type" is the only one that has an effect right now, they additionally dodged the question here by doing file type checking in Node and MIME type checking in the browser (which is mostly the same thing as servers usually infer the MIME type from the file extension).

That means my suggestion of extending ids is actually irrelevant as the loaded id is the same and they work more like module flags, similar to moduleSideEffects. So the flags would be set by the import and forwarded to the loader to do "the right thing".

The problem is now: What do we do if assertions are asymmetric i.e. differ between imports? Would then the first import decide what the flag is?

I took the time to read through the proposal and it seems they are also aware of the question and decided to not answer it as "all solutions have issues" (i.e. first wins, throw if different, clone if different).

However it seems that there is a strong willingness to not go for "clone", at least not at the moment and definitely not for JSON. That means we can make a decision here:

We go for a minimal solution: Only support type: "json" and throw for all other types and assertions as well as inconsistent assertions.
We theoretically allow for different assertions that plugins may support but figure out how to handle inconsistent imports.

In any case, we need to figure out how the assertion is forwarded to the loader.

dnalborczyk · 2021-11-09T14:06:26Z

Thanks for looking into this, wow, I did not expect it worked this way. But I could confirm: You can actually add "nonsense assertions" in Chrome safely, and they will just give you the same module instance:
// foo.js
export default {};

// main.js
import a from './foo.js' assert { foo: 'a' };
import b from './foo.js' assert { foo: 'b' };
import c from './foo.js'; 

console.log(a === b); // true
console.log(a === c); // true
I guess as "type" is the only one that has an effect right now, they additionally dodged the question here by doing file type checking in Node and MIME type checking in the browser (which is mostly the same thing as servers usually infer the MIME type from the file extension).

yeah, the assert options are optional as well:

import a from './foo.js' assert {} // works

while this seems to be ok:

import a from './foo.js' assert { foo: 'a' }
import b from './foo.js' assert { foo: 'b' }

for json it is required, when the mime-type is json, an assert option with type: 'json' is expected:

import config from "./config.json";
// Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type
// of "application/json". Strict MIME type checking is enforced for module scripts per HTML spec.

dnalborczyk · 2021-11-10T02:37:49Z

If as part of the module graph, the same module is imported once with an assertion and once without it, I assume the runtime will treat them as two separate modules, is that right?

update: node.js removed assertion-less json module imports: nodejs/node#40758 (comment) in v17.1.0 (--experimental-json-modules is still required with assertions)

lukastaegert · 2022-10-13T04:21:28Z

This is now part of Rollup 3

feat: add support for import assertion syntax

e2ec5b1

dnalborczyk changed the title ~~feat: add support for import assertion syntax~~ feat: add support for import assertions Nov 6, 2021

jogibear9988 mentioned this pull request Dec 21, 2021

esbuild should add "assert" option to css imports evanw/esbuild#1871

Closed

lukastaegert force-pushed the master branch 7 times, most recently from eca6a6c to 71d20c9 Compare September 21, 2022 20:07

lukastaegert closed this Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for import assertions #4267

feat: add support for import assertions #4267

dnalborczyk commented Nov 6, 2021 •

edited

codecov bot commented Nov 6, 2021

lukastaegert commented Nov 7, 2021 •

edited

lukastaegert commented Nov 7, 2021 •

edited

dnalborczyk commented Nov 9, 2021 •

edited

dnalborczyk commented Nov 9, 2021 •

edited

lukastaegert commented Nov 9, 2021

dnalborczyk commented Nov 9, 2021

dnalborczyk commented Nov 10, 2021 •

edited

lukastaegert commented Oct 13, 2022

feat: add support for import assertions #4267

feat: add support for import assertions #4267

Conversation

dnalborczyk commented Nov 6, 2021 • edited

Description

current json support:

import assertions/json modules

codecov bot commented Nov 6, 2021

Codecov Report

lukastaegert commented Nov 7, 2021 • edited

lukastaegert commented Nov 7, 2021 • edited

dnalborczyk commented Nov 9, 2021 • edited

dnalborczyk commented Nov 9, 2021 • edited

lukastaegert commented Nov 9, 2021

dnalborczyk commented Nov 9, 2021

dnalborczyk commented Nov 10, 2021 • edited

lukastaegert commented Oct 13, 2022

dnalborczyk commented Nov 6, 2021 •

edited

lukastaegert commented Nov 7, 2021 •

edited

lukastaegert commented Nov 7, 2021 •

edited

dnalborczyk commented Nov 9, 2021 •

edited

dnalborczyk commented Nov 9, 2021 •

edited

dnalborczyk commented Nov 10, 2021 •

edited