fix(index): correct order of CSS imports

[jarandmi]

This PR contains a:

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

Motivation / Use-Case

Modules with undefined ID will now be sorted to the bottom of array, and imported after the once with and ID

Breaking Changes

Additional Info

[sokra]

Test case missing

[sokra]

I assume this happens when chunks are shared between multiple chunk groups. This can happen when using splitChunks with a constant name. Maybe it can be fixed by using all chunk groups in chunk.groupsIterable. But make sure to create a test case to reproduce this bug first.

[jarandmi]

@sokra Yes, you're right. Thats when it happens. I'm not so familiar with Webpack and groupsIterable, but will try to dig into it.

[sokra]

Here only the first chunkGroup is used. Maybe this need to be reconsidered. See also comment above.

[jarandmi]

@sokra : I have created a test case: https://github.com/jarandmi/scss-order-bug
Try to run yarn start:dev

The order the style chunks is return is:

module.id | chunkGroup.getModuleIndex2(module)
1 | 24
3 | 28
4 | 30
5 | 33
6 | undefined
2 | 26
7 | undefined

I think the problem has too do with sass, and the `@import "../variables";``
When i use this the order of the import is in wrong order.

The order will also be different in develop and production build. And none of them seem right.

My suggestion is to make a sort function that order them in the correct way.
It should be:

module.id | chunkGroup.getModuleIndex2(module)
1 | 24
2 | 26
3 | 28
4 | 30
5 | 33
6 | undefined
7 | undefined

[jarandmi]

It will also be fixed if i sort by module.id, instead of chunkGroup.getModuleIndex2(module)

modules.sort( (a, b) =>  a.id <  b.id  ? -1  : 1 );

[rowanoulton]

Hi there, dropping in to say we're experiencing this problem as well. chunkGroup.getModuleIndex2 will return undefined for any module that isn't part of the graph of the chosen chunk group. Depending on the location of these undefined indexes, the sort may or may not work properly.

Fixing the comparator function to sort undefined to the bottom is a good step. I would also encourage we alert the engineer to the situation (eg. console.warn) as it'll save people time they would otherwise spend investigating why their CSS order is wrong.

That being said, I think there's a bigger problem to consider: when sharing CSS between multiple chunk groups, we don't know for sure what the correct order is. Consider the original problem (#130):

a.js

import './a.css';
import './b.css';

b.js

import './b.css';
import './a.css';

If we want to share CSS between these two chunks, we have to decide how to order a.css and b.css. This is a problem even if we fix the sorting of undefined. If we use the first entrypoint as the source of truth (as it's currently implemented) our CSS order will be implicitly dependent on entrypoint order. This is not obvious and, I think, fiendishly difficult to debug.

I'm not sure what the proper solution is. It seems to me that the current configuration doesn't provide enough information to correctly judge what CSS order is correct — at least in the case where it's shared across multiple chunks. The remedy might be more configuration, making a "best guess", or handing off the responsibility to the user:

plugins: [
  new MiniCSSExtractPlugin({
    sharedSortComparator: (a, b) => {
      /* .. your own logic to determine CSS order ... */
    }
  })
]

Personally I'm in favor of the latter as this solves our use case but I'd be interested to hear your thoughts on this @jarandmi @sokra. Happy to work on an implementation too. For now we've forked the plugin to revert to the old sort method (ie. using module.index2 instead of chunkGroup.getModuleIndex2).

Possibly related issues: #188, #202, #236

[sokra]

Could you check if #246 fixes your problem?