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

CSS import order does not match the browser #5840

Open
evanw opened this issue Feb 14, 2021 · 6 comments
Open

CSS import order does not match the browser #5840

evanw opened this issue Feb 14, 2021 · 6 comments

Comments

@evanw
Copy link
Contributor

evanw commented Feb 14, 2021

🐛 bug report

I am the main developer for the esbuild. While investigating import order for esbuild, I discovered that CSS has unintuitive import order. Import order in CSS does not behave like JavaScript. In JavaScript, import order is a depth-first traversal where each file is evaluated in the first location it is encountered in depth-first order. InBut in CSS, import order appears to be a depth-first traversal where each file is evaluated in the last location it is encountered in depth-first order. Using the first location instead of the last location when bundling CSS leads to behavior that doesn't match the browser.

I'm telling you about this since I figured you may not already be aware of this and might find it interesting. It's up to you whether you want to change this or not (I don't need it changed). Feel free to close this issue if you don't want to change this. The way CSS works is unfortunate and unintuitive so I could see a potential argument for deviating from CSS semantics even though it means your code will behave differently than the browser. See postcss/postcss-import#211 for an example. Someone filed a bug against postcss-import arguing that the correct but unintuitive behavior was a bug. A maintainer agrees, and changes it to the incorrect but intuitive behavior.

🎛 Configuration (.babelrc, package.json, cli command)

Command:

parcel build entry.css

🤔 Expected Behavior

I believe the output should look like this:

body{font-size:20px;font-size:10px;line-height:10px;line-height:20px}

Specifically, the font size should be 10px to match how the browser works.

😯 Current Behavior

This is what Parcel gives instead:

body{font-size:10px;line-height:10px;font-size:20px;line-height:20px}

Specifically, the font size is 20px instead of 10px (esbuild also currently has this bug, although I will be fixing this at some point).

💁 Possible Solution

CSS is specified such that each @import is supposed to behave like #include and be literally expanded inline. But that would lead to massive code bloat with exponential expansion for deeply nested @imports, and would also cause infinite loops for cycles in the import graph. There is a postcss-import flag called skipDuplicates that follows the CSS specification in this way, but it leads to code bloat and causes infinite loops with cycles.

I believe the real solution is to order CSS imports in reversed reverse preorder instead of in postorder. In other words, evaluate a CSS file once but in the last location in depth-first traversal order instead of the first location. This is how I plan to implement this in esbuild.

🔦 Context

Context: evanw/esbuild#465 (comment)

💻 Code Sample

Inputs:

  • entry.css

    @import "./file1.css";
    @import "./file2.css";
  • reset.css

    body {
      font-size: 10px;
      line-height: 10px;
    }
  • file1.css

    @import "./reset.css";
    body {
      font-size: 20px;
    }
  • file2.css

    @import "./reset.css";
    body {
      line-height: 20px;
    }

🌍 Your Environment

Software Version(s)
Parcel 2.0.0-beta.1
Node v15.2.0
npm/Yarn 7.5.2
Operating System macOS 10.15.7
@mischnic
Copy link
Member

Interesting. So it should be something like this instead (though circular imports also have to be handled correctly)

let assetOrder = new Set();
bundle.traverseGraphAndRevisitNodes({
	exit(asset) {
		assetOrder.delete(asset);
		assetOrder.add(asset);
	}
})

Another case of "CSS is rather strange" that was reported to us (while you're at it 😄 ): #5484

@evanw
Copy link
Contributor Author

evanw commented Feb 14, 2021

Another case of "CSS is rather strange" that was reported to us (while you're at it 😄 ): #5484

Yeah. I'm discovering that bundling CSS correctly is really complicated. CSS is specified so that all @import rules must come first, but that causes this subtle problem: consider a case where you have @imports to external CSS (outside of the bundle) that come after @imports to internal CSS (inside the bundle):

/* entry.css */
@import "./internal.css";
@import url('https://fonts.googleapis.com/icon?family=Material+Icons');
.someDiv { ... }
/* internal.css */
.someDiv2 { ... }

Preserving the original order of the CSS during bundling means that you cannot inline all of the internal CSS into a single file like this:

/* This is different than the original CSS evaluation order */
@import url('https://fonts.googleapis.com/icon?family=Material+Icons');
.someDiv2 { ... }
.someDiv { ... }

That would cause it to be evaluated after the external CSS instead of before. It's impossible to bundle this into a single file while preserving the original order. You technically have to generate another chunk for the internal CSS that comes before the external CSS and then continue to @import it even though it's in the bundle:

/* entry.css */
@import "./chunk.HASH.css";
@import url('https://fonts.googleapis.com/icon?family=Material+Icons');
.someDiv { ... }
/* chunk.HASH.css */
.someDiv2 { ... }

This is because CSS doesn't have any mechanism for abstraction so you can't declare CSS in one place and evaluate it in another place. An alternative is to put your CSS in JavaScript as strings since JavaScript does have mechanisms for abstraction. I really want to have CSS be in CSS files since it makes so much sense, but I have to admit that putting CSS in JavaScript does seem appealing and makes this stuff a lot easier.

@mischnic
Copy link
Member

Yeah, the same applies to extracting common CSS assets in local files into a shared bundle: https://github.com/devongovett/shared-css

Because

@import "./only-used-here.css";
@import "./common.css";

cannot be bundled into

@import "./common-HASH.css";

/* ...contents of only-used-here. */

but only

@import "./only-used-here-HASH.css"; /* would ideally not be in another bundle in the first place*/
@import "./common-HASH.css";

We are probably going to change the bundling approach to create these additional bundles when needed.

An alternative would be (if there's a parent HTML bundle):

<link rel="stylesheet" type="text/css" href="styles.css">  <!-- the above code -->

and then after bundling:

<link rel="stylesheet" type="text/css" href="only-used-here-HASH.css">
<link rel="stylesheet" type="text/css" href="common-HASH.css">

to have at least one fewer waterfall step. (Same for CSS imported by async JS bundles)

@evanw
Copy link
Contributor Author

evanw commented Feb 14, 2021

We are probably going to change the bundling approach to create these additional bundles when needed.

Good to know! I am considering doing this for esbuild too. Although I'm nervous that it will generate a ton of files if you have a lot of entry points since the same shared CSS may be used in slightly different orders in different entry points. But I haven't prototyped this out yet so I don't know for sure.

An alternative would be (if there's a parent HTML bundle):

Yeah Parcel is better suited to optimize this than esbuild. I love Parcel's HTML entry point feature :)

@mischnic
Copy link
Member

But I haven't prototyped this out yet so I don't know for sure.

Same here, but Webpack's CSS-in-JS (heh) is more of a last resort.

😯 Current Behavior

(Running with --no-minify of --no-optimize would not run cssnano and it should be more apparent what Parcel is actually doing)

@mischnic mischnic added the CSS label Feb 15, 2021
@evanw
Copy link
Contributor Author

evanw commented Feb 27, 2021

I just came up with a hack to work around not being able to use @import in the middle of a file. Check this out:

/* entry.css */
@import "data:text/css,\
.someDiv2 { ... }\
";
@import url('https://fonts.googleapis.com/icon?family=Material+Icons');
.someDiv { ... }

This feels gross but it does fit everything into a single file and it seems to work in the browser. I'm thinking about using this approach.

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

No branches or pull requests

2 participants