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

The order of CSS imports is not respected #465

Closed
iamakulov opened this issue Oct 16, 2020 · 6 comments · Fixed by #1293
Closed

The order of CSS imports is not respected #465

iamakulov opened this issue Oct 16, 2020 · 6 comments · Fixed by #1293

Comments

@iamakulov
Copy link

iamakulov commented Oct 16, 2020

Not sure whether this is related to #399 or a separate issue.

Test case

Consider the following project:

// index.js
import "./transitive-import.js"
import "./b.css"
import "./a.css"

console.log("foo")

// transitive-import.js
import "./c.css"
/* a.css */
body {
  color: blue;
}

/* b.css */
body {
  color: red;
}

/* c.css */
body {
  color: green;
}

Actual behavior

If you bundle the project with ESBuild:

npx esbuild ./index.js --bundle --outdir=./build

the resulting build/index.css file will look as follows:

/*** build/index.css ***/
/* a.css */
body {
  color: blue;
}

/* b.css */
body {
  color: red;
}

/* c.css */
body {
  color: green;
}

The problem here is that ESBuild changes the order of CSS modules. This changes the specificity of rules – and, ultimately, changes how the page would look. (The text would be green instead of blue.)

Expected behavior

The expected behavior is for ESBuild to bundle CSS imports in the order of their appearance in the module tree (with depth-first traversal). That’s how webpack does that (AFAIK).

You can test the webpack behavior with the following config:

package.json:

{
  "name": "test",
  "version": "1.0.0",
  "dependencies": {
    "css-loader": "5.0.0",
    "mini-css-extract-plugin": "1.0.0",
    "webpack": "5.1.3",
    "webpack-cli": "^4.0.0"
  }
}

webpack.config.js:

const MiniCssExtractPlugin = require("mini-css-extract-plugin");

module.exports = {
  entry: "./index.js",
  output: {
    path: __dirname + "/build",
    filename: "[name].js"
  },
  plugins: [new MiniCssExtractPlugin()],
  module: {
    rules: [
      {
        test: /\.css$/i,
        use: [MiniCssExtractPlugin.loader, "css-loader"]
      }
    ]
  }
};

Building the project with the above config (npx webpack) produces build/main.css that looks as follows:

body {
  color: green;
}

body {
  color: red;
}

body {
  color: blue;
}
@evanw
Copy link
Owner

evanw commented Oct 16, 2020

Thanks for reporting this. This will be good to fix. I think it's somewhat related to #399 but it's different enough that I'd like to keep this as a separate issue.

@nitzanorchestra
Copy link

nitzanorchestra commented Jan 21, 2021

I have the same problem.
Example:
I work with react, when I inject a class from parent to child the parent class should be "stronger" because it will be declared "later", but it always ends up the contrary: the child class overrides the parent, the order of classes declaration is reversed than when developing.
(btw, I use snowpack and esbuild)

@evanw
Copy link
Owner

evanw commented Feb 14, 2021

The expected behavior is for ESBuild to bundle CSS imports in the order of their appearance in the module tree (with depth-first traversal).

I initially interpreted this to mean "evaluate the CSS file once at the place of the first import in depth-first order" where each file is visited at most once. That's how JavaScript imports work and that's the way esbuild's CSS bundler currently works. But I just realized that this is incorrect. The semantics of CSS @import is more like @include than @import. From the specification:

user agents must treat the contents of the stylesheet as if they were written in place of the @import rule

I'm not sure why it's specified this way since it seems like surprising and unexpected behavior to me. But that's the way CSS actually works. So "bundling CSS imports in the order of their appearance in the module tree" would have to mean "evaluate the CSS file all over again at the place of every import in depth-first order" where the depth-first order can re-visit the same file an unbounded number of times. This can result in a combinatorial explosion of code for deep @import graphs. Here's an example of the difference:

  • 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;
    }

Running this CSS in the browser will result in a font size of 10px but bundling this CSS with esbuild and then running it in the browser will currently result in a font size of 20px. This is because esbuild currently only evaluates each file once at the location of the first @import in depth-first order instead of re-evaluating each file at every @import in depth-first order.

I wonder if it would be equivalent to just evaluate the CSS file once at the place of the last import in depth-first order. Instead of having the bundler recursively expand all @import statements, which would be crazy and cause massive code bloat. Placing the CSS last instead of first could potentially break some use cases such as e.g. importing a reset stylesheet from multiple files and then trying to override it below the import (the example above). A file further on in the bundle could re-import the reset stylesheet and erase the overrides due to CSS specificity rules. But that's how CSS works so ¯\(ツ)/¯. A CSS bundler shouldn't not deviate from CSS semantics just because the semantics are unintuitive.

I wonder what other bundlers do here. I'm also having trouble figuring out what ordering even means for dynamic JavaScript imports that then import CSS code (see #608 (comment)), but that's another topic.

Edit: Here is a quick survey of some different CSS environments for the example above:

Environment Font size Notes
browser ✅ 10px I found w3c/csswg-drafts#4287 which says "the implementation only needs to ‘insert’ them once, in the ‘furthest-down’ place they’re referenced" so browsers probably behave the way I proposed above.
webpack +
style-loader +
css-loader
✅ 10px Each file turns into a separate JS module with a CSS string instead of a single bundled CSS file. And Webpack literally does the recursive @import expansion with all of the exponential code bloat that causes. Each copy of each CSS file is an individual duplicated <style> tag in the DOM at run-time.
postcss +
postcss-import with
skipDuplicates: false
✅ 10px This is not the default behavior of postcss-import (skipDuplicates defaults to true). This also literally does the recursive @import expansion and results in exponential code bloat, although it's bundled into a single CSS file.
esbuild 🚫 20px All CSS is bundled into a single file and each input file is only present once.
parcel 🚫 20px All CSS is bundled into a single file and each input file is only present once.
postcss +
postcss-import
🚫 20px This is the default behavior of postcss-import. All CSS is bundled into a single file and each input file is only present once.

I found postcss/postcss-import#211 interesting. 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.

It's also interesting to compare how these different environments handle a cycle in the import graph:

Environment Can handle cycles Notes
browser
webpack +
style-loader +
css-loader
🚫 Webpack permits the cycle at build time but crashes at run time with ReferenceError: Cannot access 'd' before initialization.
postcss +
postcss-import with
skipDuplicates: false
🚫 This is not the default behavior of postcss-import (skipDuplicates defaults to true). When you set skipDuplicates to false, postcss hangs at 100% CPU and slowly consumes more and more memory. The build never completes.
esbuild
parcel
postcss +
postcss-import
This is the default behavior of postcss-import.

@evanw
Copy link
Owner

evanw commented Feb 15, 2021

I have been very curious why the import mechanism in CSS is so inconvenient to use. I finally found the rationale for why CSS @import is only allowed at the top of the file. Posting it here in case it's useful to someone else in the future since I had to dig for a while to find it. Someone proposed removing this restriction on the CSSWG mailing list in 2012: https://lists.w3.org/Archives/Public/www-style/2012Jan/0758.html.

Some of the reasons for this behavior from thread:

  1. The restriction that @import has to appear at the top of a file is meant, I believe, to make it easier to understand that other files are being imported. A lone @import in the middle of a file is easy to accidentally skip over for a human.

    (link)

  2. As a side note, it's also an optimization. As soon as the CSS file is received, the browser can know if there will be additionnal files to download. The more at the top the url is, the more quickly the new download can start. Also, the browser know at the first non-@import rule that he don't need to download new (css) files. This can help to make decisions for questions like "do I need to keep that TCP connection open?".

    (link)

  3. Regardless of any reasons for the original decision, this sort of change is extremely likely to produce style sheets that do not degrade gracefully on older browsers (some of which may be fixed in silicon), so it would not be safe to use on the public internet for about a decade after introduction.

    (link)

Still seems like a pretty short-sighted decision to me. Other people in the thread mention confusion about their import rules being silently ignored. And this behavior means esbuild has to potentially generate lots of extra imports when bundling to preserve CSS import order so this restriction introduces more network overhead. Oh well. The ship has sailed.

@evanw
Copy link
Owner

evanw commented Feb 27, 2021

Note: I'm just commenting about this here to document my thinking.

So the main problem is with external imports. 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 { ... }

CSS says that @import has to come first, so "hoisting" external imports to the top like this would be invalid:

/* 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. One solution would be to generate another chunk like this:

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

That's not great obviously because then the code isn't bundled anymore, at least not into a single file. This is where the extra network overhead that I was talking about earlier comes from.

I previously thought that this is the best you can do. But I just realized that there's another hack you can do instead:

/* 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, at least in modern browsers. I'm not sure what browser support looks like but it might actually be ok to have this be the primary bundling strategy. This is basically a workaround for not being able to stick @import in the middle of a file.

@Valexr
Copy link

Valexr commented May 18, 2021

🥳

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.

4 participants