-
-
Notifications
You must be signed in to change notification settings - Fork 9.1k
keep consistent css order #19012
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
base: main
Are you sure you want to change the base?
keep consistent css order #19012
Conversation
For maintainers only:
|
95e1dcd
to
e7a5253
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a test case like in your repo
// Ignore lazy import dependencies | ||
if ( | ||
directIncomingConnection.dependency instanceof | ||
ImportDependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only this? Also let's add a test case too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dynamic imports follow their own order
did I miss any other imports?
./common2.js X bytes [built] [code generated] | ||
./module_first.js X bytes [built] [code generated] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we verify is it a right change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this change (it's changing the order in the stats output)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems expected, since now we keep the order of the optimized dependencies, in this case ./first.js -> ./common.js -> ./common2.js
is optimized to ./first.js -> ./common2.js
, before the order is changed by side effects optimization, so this dependency is moved behind ./module_first.js
done - please take a look |
While trying to understand the code here, I found a case that seems not work as expected: https://github.com/ahabhgk/webpack-side-effects-optimization-keep-connections-order/tree/main/reexports In this case, |
@jantimon hello, can you please take a look at the last comment with the wrong case? |
while testing I found a bug when mixing lazy and non lazy imports I'll add a test once I can reproduce it |
@jantimon Sorry for delay, can you add a test case? |
If you need any help, just let us know :) |
@jantimon Friendly ping |
@ahabhgk you are right - this was not intended the problem is the way dependencies are tracked in webpack maybe it's easier to understand by example: import { dependency, dependency2 } from "./dependency/index.ts"; // index: 0 -> ./dependency (barrel)
import { dependency3 } from "./dependency3"; // index: 1 -> ./dependency3 (barrel)
export function component() {
dependency3(); // index: 2 -> ./dependency3.ts
dependency2(); // index: 3 -> ./dependency/dependency2.ts
dependency(); // index: 4 -> ./dependency/dependency.ts
} so with barrel files this would be
after the sorting fix it would be:
and after barrel file optimization:
I guess this is almost perfect - the only thing which is wrong is that the order in |
we spend another day of investigation and saw that a lazy import common chunk changes its child modules we can see it happening one single time in our application with 17.000 modules but we did not manage to create a mini reproduction out of it |
That's a bummer. Was this in NextJS (As I've seen you in some PRs/Issues there too) or something else? As NextJS has some issues regarding CSS order too, related to server <-> client components. If it's not, we could investigate it separately from this part, so a big chunk is already fixed with this PR :) |
Yes this was within a next.js pages router project. But we're still trying to figure out why it's happening. |
As I noticed the following in NextJS; https://github.com/Netail/repro-app-dir-css-order, but I don't think it's Webpack, else it would have been wrong in both a server as client component. The only difference is the client directive |
Should we get this in? Then, fix the remaining CSS issues in follow-up PRs, as the CSS order in the current release is completely broken with sideEffects. This should at least fix a big chunk of the current CSS issues (cc: @alexander-akait) |
What kind of change does this PR introduce?
We were struggling with the problem that the CSS ordering becomes unpredictable when webpack's side effects optimization removes barrel files (for
sideEffects: false
in package.json)Looking at webpack's buildChunkGraph.js, I traced this down to how module graph building and tree-shaking interact. When webpack finds a module marked as side effect free, it tries to optimize by skipping over barrel files (
activeState
: false`) and connecting directly to implementation files. While great for JS tree-shaking, this changes the dependency order which affects CSS since mini-css-extract-plugin relies on postOrderIndex for orderingHere's a concrete example from the reproduction repo I set up:
When building with
sideEffects: false
, webpack removes the barrel file but loses the original ordering, resulting in incorrect CSS:I've found this is happening because dependency indices get rewritten during side effects optimization
My fix in SideEffectsFlagPlugin.js maintains the original ordering by sorting the direct imports after their barrel files position
BEFORE THE FIX:
Result: CSS order is wrong because slide.ts lost its original
position relative to teaser.module.css
WITH THE FIX:
Result: CSS ordering preserved because we restored the barrel file's position
and internal ordering before removing it
I tested this fix for the reproduction repo at: https://github.com/jantimon/reproduction-webpack-css-order
With the fix the CSS order is the same as for other bundlers like Parcel or Vite
All changes happen only inside the
SideEffectsFlagPlugin
and only if a barrel file was removed by that pluginFor more information about the problem please see #18961