Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plan for import assertions and non-JS module types #3799

Closed
justinfagnani opened this issue Sep 29, 2020 · 27 comments · Fixed by #4549
Closed

Plan for import assertions and non-JS module types #3799

justinfagnani opened this issue Sep 29, 2020 · 27 comments · Fixed by #4549

Comments

@justinfagnani
Copy link

Feature Use Case

Importing JSON, CSS, and HTML according to upcoming JavaScript and HTML standards.

Feature Proposal

Import assertions are a Stage 3 TC39 proposal to allow source code to assert that an import has a specific type (among other possible assertions): https://github.com/tc39/proposal-import-assertions

Assertions are being added to unblock the JSON, CSS, and HTML modules proposals which allow importing those file type as JavaScript modules.

This means that we'll be able to import JSON, CSS and HTML into JavaScript natively, and ideally in bundlers without any specific loaders (or perhaps a "web standard loader" that implements all web-native module types?):

import styles from './styles.css' assert {type: 'css'};

// or

const styles = await import('./styles.css', {assert: {type: 'css'}});

I filed an issue on Acorn for import assertion support.

The JSON and CSS proposals are furthest along, with implementations in Chromium behind flags.

@lukastaegert
Copy link
Member

If there is an acorn plugin for import assertions, then we can include this in the Rollup build even if it is not part of acorn core. But I did not see one, so maybe this requires some community effort to drive forward.

@justinfagnani
Copy link
Author

Yeah, there doesn't appear to be an acorn plugin yet.

After that exists though, what would be the plan for Rollup support of the module types that the web platform supports? Would these be built-in?

@guybedford
Copy link
Contributor

guybedford commented Oct 7, 2020

There are basically two main cases for RollupJS - handling these module types for externals and handling these module types for RollupJS inlining.

For all types of modules, when they are externals, the import statement just stays in the chunk just the same.

For the inlining cases:

  • Wasm modules: We will have a bundle story with the module linking proposal, so that there are ways to effectively output rollup'd Wasm per chunk (a single Wasm file per chunk).
  • JSON modules: It would probably make sense to just inline the JSON as JS code.
  • CSS modules: Is there a story for combining multiple constructed stylesheets while retaining semantics? Otherwise they may just have to always remain externals. This is a shame as CSS imports today can do better than that by having one optimized CSS file per chunk.

It's worth noting that all of the above are already possible today - the syntax is if anything a restriction in forcing RolllupJS behaviour for inlining or errors at runtime.

Would be good to hear your thoughts on CSS chunking further.

@dasa
Copy link

dasa commented Oct 7, 2020

  • JSON modules: It would probably make sense to just inline the JSON as JS code.

It might be better to use JSON.parse instead if it's 10 kB or larger according to: https://v8.dev/blog/cost-of-javascript-2019#json

@justinfagnani
Copy link
Author

CSS modules: Is there a story for combining multiple constructed stylesheets while retaining semantics? Otherwise they may just have to always remain externals. This is a shame as CSS imports today can do better than that by having one optimized CSS file per chunk.

The story is to transform them to JS and do explicit new CSSStyleSheet(); sheet.replaceSync() calls. Later the story will be WebBundles, since CSS has no native multi-file support.

This is a shame as CSS imports today can do better than that by having one optimized CSS file per chunk.

The semantics are all wrong though. We need to be able to access each stylesheet individually, and importantly not apply them to the main document. The non-standard CSS imports do not support that.

@guybedford
Copy link
Contributor

The story is to transform them to JS and do explicit new CSSStyleSheet(); sheet.replaceSync() calls

Do you mean having the stylesheet strings inline in the JS to do this? That sounds like it could work just fine actually!

Would it be worth getting a RollupJS plugin going that can do this work? @tivac has previously done a lot of work in this space as well. We don't yet have any official style / css plugin AFAICT, and there's nothing to stop this sort of workflow from becoming a standard one already now even without import assertions.

@tivac
Copy link
Contributor

tivac commented Oct 8, 2020

If it's just a straight transform of CSS to JS string as the default export that seems really trivial.

Feels like you would still want some sort of .css file outputting and bundling though for SSR/progressive enhancement workflows?

@justinfagnani
Copy link
Author

Do you mean having the stylesheet strings inline in the JS to do this?

Basically. The transform wouldn't export the CSS text though, just a stylesheet.

p {
  color: red;
}

would transform to:

const stylesheet = new CSSStyleSheet();

stylesheet.replaceSync(`p {
  color: red;
}`);

export default stylesheet;

@justinfagnani
Copy link
Author

justinfagnani commented Oct 16, 2020

FYI, I just filed: w3c/csswg-drafts#5629 to propose multiple stylesheets per .css file, which would address the bundling question even without Web bundles.

@daKmoR
Copy link

daKmoR commented Jun 19, 2021

an acron plugin seems to be available for import assertions.
https://github.com/xtuc/acorn-import-assertions

In order to support import data from "./data.json" assert { type: "json" }; in rollup? what would we need to do? 🤔

somehow load this acorn plugin and it should work? or are there more steps? 🤔

@guybedford
Copy link
Contributor

@daKmoR just the import assertion plugin should be necessary. We could then possibly look at supporting JSON imports by default in RollupJS further perhaps just based on file extension.

@CxRes
Copy link

CxRes commented Aug 23, 2021

I am running into two issues as I try to use import assertions with dynamic imports:

  1. Rollup checks for a closing bracket at the end of first argument to import(), finds a comma and throws!
  2. Using the acorn-import-assertion plugin throws with error (even after commenting out the close bracket check):
TypeError: Cannot read property 'bind' of undefined
    at ImportExpression.bind (file:///D:/myproj/node_modules/.pnpm/rollup@2.56.2/node_modules/rollup/dist/es/shared/rollup.js:2425:23)

There is also a plugin (https://github.com/calebdwilliams/rollup-plugin-import-assert) for this but that tries to interpret the import path even for dynamic and import the file while bundling (all I want to achieve is for the dynamic import statement to be left alone)!

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Oct 26, 2021

  • JSON modules: It would probably make sense to just inline the JSON as JS code.

It might be better to use JSON.parse instead if it's 10 kB or larger according to: v8.dev/blog/cost-of-javascript-2019#json

@dasa I don't think rollup should do that automatically depending on file size. probably better to opt in or out as you can do with any file/module using external: https://rollupjs.org/guide/en/#external

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Oct 26, 2021

I'm thinking the json import assertion functionality should be pulled into rollup core now, as it appears to be part of the javascript language, although it seems that the host decides what to do with the imported value.

another thing is also that the spec disallows named imports and expects a syntax error being thrown when type="json"

import { foo } from './foo.json' assert { type: 'json' }

currently the json plugin is required to import json and it allows named imports, when plain "objects" are being imported, and a property with the same name as the named import exists, otherwise it throws (also throws for any other valid json: [], "string", true, 123).

I would assume that the syntax would be disallowed when using assertion syntax, and I think it should probably be retired altogether, as named imports are not allowed anywhere [for json] (as far as I know), even in node.js without using assertions.

@dasa
Copy link

dasa commented Oct 26, 2021

  • It would probably make sense to just inline the JSON as JS code

@dnalborczyk I'm only referring to if it's embedded, then how it's embedded. According to this blog post, surprisingly it can be faster in some cases to embed it as a string and use JSON.parse vs. "just inline the JSON as JS code".

@dnalborczyk
Copy link
Contributor

I'm only referring to if it's embedded, then how it's embedded. According to this blog post, surprisingly it can be faster in some cases to embed it as a string and use JSON.parse vs. "just inline the JSON as JS code".

ah, ok, gotcha, that makes sense. I don't know where my mind has been 😄 for some reason I thought you suggested omitting inlining altogether.

@thepassle
Copy link

thepassle commented Jan 19, 2022

I recently spent some time to add support for import assertions and CSS/JSON modules to our application at work, I figured I'd write down the experience in case anybody is looking to do the same, and hopefully it'll help rollup in supporting them better.

I've put it in a separate gist, because it got a bit long and didnt want to pollute this issue too much.

https://gist.github.com/thepassle/d46a7cabf2dd15ba3bb43397430e182f

Most notably and relevant to this thread is that:

  • Rollup will crash on the css syntax after adding the acorn plugin for import assertions, currently the only realistic option seems to be to transform the CSS to JS, which feels sort of wrong
  • Rollup will transform the imports incorrectly (even after marking them as external), e.g.:
import styles from './styles.css' assert { type: 'css' };

Always becomes:

import styles from './styles.css';

See the gist for more info. Hopefully it's helpful.

@mathe42
Copy link

mathe42 commented Jan 26, 2022

Why not just say - as it is currently - rollup supports only js out of the box and change the resolveID hook to get an extra argument with the contents of assertion this would allow the following in plugins:

Assume style.css contains a class "red".

import {red} from './styles.css' assert { type: 'css', module: true };

The proposal is not limited to a type field (that is only what other proposals currently use / need)

The resolveId hook could get the folowing type

type resolveId = (source: string, importer: string | undefined, options: {isEntry: boolean, custom?: {[plugin: string]: any}, assertions: Record<string, any>) => string | false | null | {id: string, external?: boolean | "relative" | "absolute", moduleSideEffects?: boolean | "no-treeshake" | null, syntheticNamedExports?: boolean | string | null, meta?: {[plugin: string]: any} | null}

this should be a small but powerfull change in the syntax. A lot of plugins could change there syntax to a specific assertion. For example the comlink-plugin you could use

import worker from './myworker.js' assert { type: 'comlink' };

For rollup we don't need the assert { type: 'json' } as the filename contains this information. And if a user don't want the default import syntax a plugin can change that but only need to rely on the filename. So the assert syntax can be used to inform a plugin wich transforms or loader you want.

@thepassle
Copy link

The proposal is not limited to a type field (that is only what other proposals currently use / need)

I think its a bad idea to have to adhere to custom build-time conventions/syntax to support native browser functionality. Plugins will definitely start to abuse the assertion, but I dont think it should be the default behavior.

@dmail
Copy link
Contributor

dmail commented Jan 26, 2022

For rollup we don't need the assert { type: 'json' } as the filename contains this information

A browser understands the following and I hope rollup too one day (for now I have a fat custom plugin to allow that):

import style from 'https://cdn.com/styles.css' assert { type: 'css' };

In that scenario code fetching the ressource still have to check the content type received from server is "text/css" as a browser would.

@justinfagnani
Copy link
Author

justinfagnani commented Jan 26, 2022

For rollup we don't need the assert { type: 'json' } as the filename contains this information

To match browser behavior you absolutely do need the assertion. If you assume standard mime-types and someone writes:

import * as foo from 'foo.js' assert {type: 'json'};

Then Rollup should fail, since that's what the browser would do.

@justinfagnani
Copy link
Author

import {red} from './styles.css' assert { type: 'css', module: true };

This would be bad to support for two reasons:

  1. The file with the import would only work in Rollup, not in browsers. Rollup should strive to build features on browser-standard syntax so that people are writing real supported-JS and not "Rollup JS". Projects should be able to work in the browser and then be optimized by Rollup with no behavior change.
  2. Import assertions should not be used to alter the import, only assert something about the import and fail if the assertion is violated. This is clearly stated in the spec and is important for the way the module cache works. Things will fail if you try to use assertions for transforms but add different assertions at different import sites. There are other JS proposals for adding ways to transform the import.

@mathe42
Copy link

mathe42 commented Jan 26, 2022

I agree that my idea was bad :D.

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Jan 26, 2022

For rollup we don't need the assert { type: 'json' } as the filename contains this information

To match browser behavior you absolutely do need the assertion. If you assume standard mime-types and someone writes:

import * as foo from 'foo.js' assert {type: 'json'};

Then Rollup should fail, since that's what the browser would do.

the same is currently true for node.js (albeit import assertions still sitting behind a flag) and deno.

@lukastaegert
Copy link
Member

Initial support in #4646

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #4646 as part of rollup@3.0.0-8. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.0.0-8 or npm install rollup@beta. It will likely become part of a regular release later.

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #4646 as part of rollup@3.0.0. You can test it via npm install rollup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.