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

Duplicate imports are resolved in the wrong order #462

Closed
evanw opened this issue Jun 23, 2021 · 2 comments · Fixed by #535
Closed

Duplicate imports are resolved in the wrong order #462

evanw opened this issue Jun 23, 2021 · 2 comments · Fixed by #535

Comments

@evanw
Copy link

evanw commented Jun 23, 2021

This plugin uses a different import order than the browser, which can lead to visual discrepancies when CSS is bundled vs. unbundled. I believe this is a regression and that it was caused by #211. The behavior used to be correct but was broken in that change, and is now incorrect.

Here's an example (note that the file common.css is being imported twice):

$ cat common.css
body { background: white; color: black; }

$ cat a.css
@import "./common.css";
body { background: red; }

$ cat b.css
@import "./common.css";
body { color: red; }

$ cat entry.css
@import "./a.css";
@import "./b.css";

$ cat index.html
<link rel="stylesheet" href="entry.css">
Some example text

When you view index.html in a browser, you should see a white background and red text. However, when bundled with PostCSS, the entire page is red.

Here's the script I used to bundle with PostCSS:

const css = require('fs').readFileSync('entry.css', 'utf8')
require('postcss')().use(require('postcss-import')({
})).process(css, {
  from: 'entry.css',
}).then(result => {
  console.log(result.css)
})

The incorrect behavior can be "fixed" by adding skipDuplicates: false to the postcss-import options, but that's not great because it makes the output bigger unnecessarily. This option is also dangerous because skipDuplicates: false can cause postcss-import to infinite-loop and hang if the import graph has cycles.

The way to fix all of these problems and have postcss-import match what the browser does is to only respect the last duplicate @import statement instead of only respecting the first duplicate @import statement.

@RyanZim
Copy link
Collaborator

RyanZim commented Jun 30, 2021

Sorry for the slow reply here; reproduced. However, postcss-import has had this incorrect behavior for a long time; changing it completely would cause a massive shockwave in the ecosystem. Also, depending what plugins you're using, for example https://github.com/postcss/postcss-simple-vars, you want the current behavior, so your imported files are always at the top of the file.

At this point, I think our only option is to introduce an option to specify whether you want to use the first or last duplicate import. Perhaps roll skipDuplicates into this as a new single option for specifying duplicate handling. Default would be debatable.

@marat-gainullin
Copy link

Indeed the behaviour is different from browser's. But the browser's behaviour IMHO is not the case for the task. Since postcss-import is a tool for bundling libraries with their complex dependencies recusively in applications, the behaviour when the first occurrence of imported file is included first is desired. Browser's behaviour is to give a favour to last included CSS. If a browser meets something twice it loads it twice. That is why the behaviour of browser differs. As for me building a bundle of a project with complex dependencies is just another task, then the browser's. I believe that CSS files should be included only with the first reference because they should be coupled with the code of the library they come from. Moreover, all dependants are free to override the libraries' rules partially. This aligns with that rule, that something more specific overrides something that is more general. The most deep dependency should be included in a bundle first just because of the nature of libraries.

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

Successfully merging a pull request may close this issue.

3 participants