Chunks are not split properly with experimentalUseImportModule

koggdal commented 2 years ago

Bug report

Chunks aren't split properly for CSS when using mini-css-extract-plugin@>=2.4.0 (or older together with experimentalUseImportModule: true).

This is mainly the same as https://github.com/webpack-contrib/mini-css-extract-plugin/issues/850 but that was closed.

Actual Behavior

Code gets folded into the main bundle.

Expected Behavior

Code gets split into the right chunks as specified in the config.

How Do We Reproduce?

Check out this repo with examples for when it's working, when it's not working, and a potential fix.

https://github.com/koggdal/sample-mini-css-extract-plugin-issue-850

Read the README in that repo for more details.

Please paste the results of `npx webpack-cli info` here, and mention other relevant information

  System:
    OS: macOS 12.3.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 27.37 GB / 64.00 GB
  Binaries:
    Node: 16.14.2 - ~/.nvm/versions/node/v16.14.2/bin/node
    Yarn: 1.22.18 - ~/.yvm/.yarn/shim/yarn
    npm: 8.5.0 - ~/.nvm/versions/node/v16.14.2/bin/npm
  Browsers:
    Chrome: 100.0.4896.127
    Firefox: 99.0.1
    Safari: 15.4
  Packages:
    css-loader: ^6.7.1 => 6.7.1 
    webpack: ^5.72.0 => 5.72.0 
    webpack-cli: ^4.9.2 => 4.9.2

alexander-akait commented 2 years ago

Hellom sorry for delay, can you try:

test: (module) => {
  return /\/my-feature($|\/)/.test(module.identifier());
},

Because now we execute modules with CSS on Node.js side we lose context, anyway, context is not the good solution, some plugins and loaders can change them too, better to use module.identifier(), it is unuque, it will not changed and have a path to original file

koggdal commented 2 years ago

Thanks, and sorry for delay on my side too!

I tried this, and it does work fine in the sample repo I set up, but only because of the specific test being done. In our real case we were also testing for things around node_modules, and then it gets trickier because module.identifier() contains paths to the loaders as well.

# module.identifier()
/workspace/node_modules/mini-css-extract-plugin/dist/loader.js!/workspace/node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[2].use[1]!/workspace/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[2].use[2]!/workspace/node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[2].use[3]!/workspace/components/MyComponent/MyComponent.scss

# module.context (with experimentalUseImportModule: false)
/workspace/components/MyComponent

# module.context (with experimentalUseImportModule: true)
/workspace

While we could solve this in our case by having a regex that avoids the individual loaders, it would be preferable to not maintain a list of any loader names in there, especially since this fails silently and just results in less code splitting and larger bundles.

I suppose we could also do module.identifier().split('!') and check against the last path there in the resulting array, but that feels less than ideal to have to do string manipulation like that.

While context may not be the best solution in your opinion, would there be downsides from making the change I suggested in https://github.com/koggdal/mini-css-extract-plugin/commit/a501265d717e143dc98b2760e93e9d2aeb85bb62 ? And for me to understand this point better, what is module.context supposed to be a path to?
To avoid having a complex regex containing loader names or do string manipulation, what do you recommend us checking for?

Thank you! 🙏

alexander-akait commented 2 years ago

@vankop What do you think? We can change it on context, but I am afraid it can be breaking change for other developers...

vankop commented 2 years ago

module.context could be null.. not sure that this is good solution. ( maybe module.resource )

However, I didn't get why we loose context in case of experimentalUseImportModule: true @alexander-akait ? ( maybe this should be fixed )

alexander-akait commented 2 years ago

@vankop It was implemented by @sokra before, so I don't know :smile:

koggdal commented 2 years ago

As module.resource seemed like a good idea (and I found that the sample in the docs also uses this property), I tried it, but it seems that is undefined for the CSS resources, so I can't check for that either.

We can change it on context, but I am afraid it can be breaking change for other developers...

On breakage here, does this refer to the people who used experimentalUseImportModule: true before 2.4.0? I of course don't want breakage for anyone, but this issue is exactly about breakage coming from making experimentalUseImportModule: true the default in 2.4.0, so fixing module.context to what it was before 2.4.0 would unbreak the non-experimental mainline release, and potentially (?) break for very few users of the experimental feature that rely on the new value for it.

If we find it difficult to fix module.context, any other suggestions for what we can check for are appreciated! 🙏

alexander-akait commented 2 years ago

Let's wait @sokra answer

koggdal commented 2 years ago

Hi @sokra! 👋

I totally understand everyone is busy and I have no expectation of a speedy answer or support here for an OSS project, so see this as just a friendly reminder in case this was lost somewhere :) Thanks for all the great work! ❤️

webpack-contrib / mini-css-extract-plugin