Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jantimon
Copy link
Contributor

@jantimon jantimon commented Nov 26, 2024

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 ordering

Here's a concrete example from the reproduction repo I set up:

// @libraries/teaser/src/teaser.ts
import { CarouselButton } from '@segments/carousel'; // via barrel file
import styles from './teaser.module.css';

When building with sideEffects: false, webpack removes the barrel file but loses the original ordering, resulting in incorrect CSS:

.teaser { ... }  /* Should be last */
.carousel { ... } /* Should be first */ 

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:

Initial import order in parent module:
index: 0 -> @segments/carousel/index.ts (barrel file)
index: 1 -> ./teaser.module.css

After tree-shaking removes barrel file:
index: 0 -> @segments/carousel (inactive)
index: 1 -> ./teaser.module.css
index: 2 -> @segments/carousel/slide.ts  (lost original order from barrel)

Result: CSS order is wrong because slide.ts lost its original
position relative to teaser.module.css

WITH THE FIX:

Initial import order in parent module:
index: 0 -> @segments/carousel/index.ts (barrel file)
index: 1 -> ./teaser.module.css

After tree-shaking + fix:
index: 0 -> @segments/carousel (inactive)
index: 1 -> @segments/carousel/slide.ts  (sorted next to its barrel file)
index: 2 -> ./teaser.module.css

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 plugin

For more information about the problem please see #18961

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@jantimon jantimon force-pushed the side-effects-optimization-css-order branch from 95e1dcd to e7a5253 Compare November 27, 2024 11:07
Copy link
Member

@alexander-akait alexander-akait left a 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
Copy link
Member

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

Copy link
Contributor Author

@jantimon jantimon Dec 10, 2024

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]
Copy link
Member

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?

Copy link
Contributor Author

@jantimon jantimon Dec 10, 2024

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)

Copy link
Contributor

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

@jantimon
Copy link
Contributor Author

Let's add a test case like in your repo

done - please take a look

@alexander-akait
Copy link
Member

/cc @ahabhgk @h-a-n-a Will be great if you look at this (feel free to ping other developers), I think it will be useful for rspack too, so I need a couple extra eyes, thank you

@ahabhgk
Copy link
Contributor

ahabhgk commented Feb 6, 2025

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, dependency/index.js re-exports ./dependency2 and ./dependency, so the order of their css should be [dependency2.css, dependency.css], but the result is [dependency.css, dependency2.css]

@valeryq
Copy link

valeryq commented Feb 18, 2025

@jantimon hello, can you please take a look at the last comment with the wrong case?
It would be really helpful if this issue can be fixed. Thank you!

@jantimon
Copy link
Contributor Author

while testing I found a bug when mixing lazy and non lazy imports

I'll add a test once I can reproduce it

@alexander-akait
Copy link
Member

@jantimon Sorry for delay, can you add a test case?

@Netail
Copy link

Netail commented Mar 28, 2025

If you need any help, just let us know :)
Would love to get this fixed for our design system 😅

@alexander-akait
Copy link
Member

@jantimon Friendly ping

@jantimon
Copy link
Contributor Author

jantimon commented Apr 4, 2025

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, dependency/index.js re-exports ./dependency2 and ./dependency, so the order of their css should be [dependency2.css, dependency.css], but the result is [dependency.css, dependency2.css]

@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

  • index: 0 -> ./dependency (barrel)
  • index: 1 -> ./dependency3 (barrel)
  • index: 2 -> ./dependency3.ts
  • index: 3 -> ./dependency/dependency2.ts
  • index: 4 -> ./dependency/dependency.ts

after the sorting fix it would be:

  • index: 0 -> ./dependency (barrel)
    • index: 3 -> ./dependency/dependency2.ts
    • index: 4 -> ./dependency/dependency.ts
  • index: 1 -> ./dependency3 (barrel)
    • index: 2 -> ./dependency3.ts

and after barrel file optimization:

  • index: 3 -> ./dependency/dependency2.ts
  • index: 4 -> ./dependency/dependency.ts
  • index: 2 -> ./dependency3.ts

I guess this is almost perfect - the only thing which is wrong is that the order in import { dependency, dependency2 } is ignored

@jantimon
Copy link
Contributor Author

jantimon commented Apr 4, 2025

@jantimon Sorry for delay, can you add a test case?

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

@Netail
Copy link

Netail commented Apr 4, 2025

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 :)

@Mad-Kat
Copy link

Mad-Kat commented Apr 4, 2025

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.

@Netail
Copy link

Netail commented Apr 4, 2025

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 and our primary goal is to fix the CSS order issues with this PR 😅 . 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

@Netail
Copy link

Netail commented May 7, 2025

Friendly ping @jantimon / @Mad-Kat :)
Any update?? 👀

@Netail
Copy link

Netail commented Jun 10, 2025

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)

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

Successfully merging this pull request may close these issues.

7 participants