Race conditions when processing multiple CSS files

romainmenke commented 1 year ago

Bug report

see : https://github.com/csstools/postcss-plugins/issues/721#issuecomment-1322181614

PostCSS Loader has race conditions with certain plugins.

Actual Behavior

I would expect Webpack + PostCSS Loader to mimic how CSS is "imported" in a browser.

file 1 has global objects like --foo and @custom-media
file 2 uses these global objects

A PostCSS plugin glues it all together and provides fallbacks :

stores global objects from file 1 in a shared object
shared object can be used when processing file 2

In reality this is one big race condition because files are processed in parallel.

Expected Behavior

I would expect some way to avoid race conditions while at the same time sharing state.

Is there configuration that needs to be set?
Do we need to write PostCSS plugins in a different way.

How Do We Reproduce?

https://github.com/csstools/postcss-plugins/pull/723

clone that repo
git checkout webpack-race-condition-repro--adaptable-striped-rocket-frog-0e8f20596a
cd ./e2e
npm ci
cd ./webpack/postcss-loader
npm run build
inspect e2e/webpack/postcss-loader/dist/main.css

Repeat npm run build and watch the output change.

romainmenke commented 1 year ago

The issue can also be viewed like this :

PostCSS loader processes and imports files in a mixed order. I would expect it to import first and run all PostCSS plugins on the bundled result.

If it does this, then there would only be one plugin instance operating on the whole bundle and there would be no race condition.

Is there a way to achieve that?

romainmenke commented 1 year ago

Is postcss-import the best way to resolve this issue?

If it is, we can add this to the documentation of each plugin that is likely to have this issue.

alexander-akait commented 1 year ago

I don't see any problem with postcss-loader here, even more you will have the same problem with rollup/esbuid/vite/etc

alexander-akait commented 1 year ago

In reality this is one big race condition because files are processed in parallel.

yes, you can't rely on the order, even more you can have circular dependies, you need weak graph here to handle such cases and store AST/variables of each module, also don't forget about layer(...) stuff, but it still invalid approach and can't solve problems

or you can run webpack in sync mode and perf will lost

alexander-akait commented 1 year ago

But again, no CSS import in JS (ESM) files (and will not be in future, it is bundle stuff with a lot of limitations), order JS modules and CSS files are different things (and have different logic), and you should avoid using:

import './media-queries.css';
import './media-queries-2.css';
import './styles.css';

because each module is independent (ESM spec says it cleary).

You should import CSS inside CSS, i.e.

@import url("./media-queries.css");
@import url("./media-queries-2.css");

:root {
    --c: hsl(50deg 50% 50%);
}

.a {
    color: var(--c);
}

@media (--tablet-up) {
    .a {
            color: pink;
        }
}

@media (--tablet-down) {
    .a {
        color: hotpink;
    }
}

Even more, in this case style.css have dependecies, so bundler can handle it (and don't rebuild whole files and better cache)

romainmenke commented 1 year ago

I don't see any problem with postcss-loader here

I agree :)

I think most users don't consider this aspect when writing code and create bugs for themselves. They expect things to be both fast/parallel and magically communicate stateful things.

This only came up because we altered a PostCSS plugin to make sure that state was reset with each invocation.

You should import CSS inside CSS, i.e.

I asked the reporter to do this, but they didn't like that they would have to type @import url(...) 🤷

I agree that there is no issue here. Thank you for sharing your point of view and insights into how Webpack/PostCSS loader work.

alexander-akait commented 1 year ago

We can document it (it seemed to me that we have already done this and more than once)

I asked the reporter to do this, but they didn't like that they would have to type @import url(...) shrug

I don't think this is the place to discuss whether we want or don't want, there is a specification that clearly tells us how to handle ESM modules, so to speed bundling we handle them in such mode. It will be extremely strange to block the entire graph when we find CSS (note - webpack don't know type of files (except JS), we configure it), taking into account the fact that in the specification there is no such thing as CSS import (except import style from "style.css" assert { type: 'css' }, but it is the other story).

And extracting such cases is the problem too, because you can have: A -> B, B -> A and who is main here :confused:

alexander-akait commented 1 year ago

frankly, if you have a global CSS - the best way is using additional entry - you will have better perf, avoid mixing JS and CSS (no such problems, as I written above), faster rebuild and you can even use dependOn on another CSS chunks (and do shared styles)

romainmenke commented 1 year ago

We can document it (it seemed to me that we have already done this and more than once)

We don't document this yet on the PostCSS plugins, but we will add this.

Yup I think a lot is complicated by mixing JS and CSS and getting confused about ESM imports works vs. how CSS works in a whole document in a browser.

People want to do everything through JS these days but still expect those two completely different rule sets to remain true.

I personally don't use Webpack and CSS/PostCSS this way and at first I had incorrect expectations based on what was reported to me.

webpack-contrib / postcss-loader