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

fix(order): use correct order when multiple chunk groups are merged #246

Merged
merged 1 commit into from Aug 21, 2018

Conversation

sokra
Copy link
Member

@sokra sokra commented Aug 20, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

see #242

Order of modules is incorrect/undefined when merging multiple chunk groups into a single chunk.

This PR also emits a warning when order is conflicting between chunk groups.

Breaking Changes

bugfix

Additional Info

@jarandmi
Copy link

Yes! This fix my problem. But i'm not sure if we should warn in the console. Do we want the user to take any action to fix this, if shared chunks is used in different order?

@sokra
Copy link
Member Author

sokra commented Aug 21, 2018

Do we want the user to take any action to fix this, if shared chunks is used in different order?

Yes, the user should make sure to reorder the imports to avoid conflicting order.

When a warning is emitted the code doesn't define multiple possible orderings. We choose one possible order here, but some other factors could make us choosing another ordering (i. e. adding entrypoints or import()s). The new order could result in a broken CSS. Conclusion: A conflicting order makes your app fragile and should be avoided.

@alexander-akait alexander-akait merged commit c3b363d into master Aug 21, 2018
@alexander-akait alexander-akait deleted the bugfix/order branch August 21, 2018 13:11
@jarandmi
Copy link

I agree, but it will be very hard to use reusable components (like in react) this way.

Example:

Text.js
import "./textStyle.css"

ComponentOne
import "./componentOneStyle.css"
import Text from "./Text.js"

ComponentTwo
import "./ComponentTwo.css"
import Title from "./Title.js"
import Text from "./Text.js"

I think it will be almost impossible to order this the right way if a component is reused in many other components. What do you think?

@robwierzbowski
Copy link

robwierzbowski commented Jan 7, 2019

We're finding warnings in our application since we started optimizing with SplitChunksPlugin, and having a hard time fixing them. Just starting to dig through the code here. Does the comparison function check index of the import, like this:

file 1

import "./a.css"; // 1
import "./b.css"; // 2
import "./d.css"; // 3

file 2

import "./a.css"; // 1
import "./b.css"; // 2
import "./c.css"; // 3
import "./d.css"; // 4, fire warning because d is not index 3, although it's still after a and b

or comparative order, like this?

file 1

import "./a.css"; 
import "./b.css"; 
import "./d.css";

file 2

import "./a.css"; // before b and d
import "./b.css"; // after a, before d
import "./c.css"; // new
import "./d.css"; // after a and b

@robwierzbowski
Copy link

The way we use modules in practice, I'm not sure we'll be able to get rid of these warnings. We have pages that use many of the same modules, but imported via different means. For an extremely reduced example:

pageOne
  import header
  import product1
  import footer
    import responsiveImage

pageTwo
  import headerExperiment
    import header
    import headerVariant
      import responsiveImage
  import product1
  import footer
    import responsiveImage

This doesn't have any practical impact since we use CSS modules, making our CSS non-order dependent. Thoughts? Maybe we can silence warnings for any CSS that uses generated unique scoping?

@robwierzbowski
Copy link

After reading some of the comments in #250 I think my team will silence warnings. Apologies for the noise.

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.

None yet

5 participants