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

Chunks are not split properly with experimentalUseImportModule #933

Open
koggdal opened this issue Apr 21, 2022 · 8 comments
Open

Chunks are not split properly with experimentalUseImportModule #933

koggdal opened this issue Apr 21, 2022 · 8 comments

Comments

@koggdal
Copy link

koggdal commented Apr 21, 2022

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

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
Copy link
Author

koggdal commented May 4, 2022

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 koggdal@a501265 ? 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
Copy link
Member

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

@vankop
Copy link
Contributor

vankop commented May 4, 2022

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

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

@koggdal
Copy link
Author

koggdal commented May 5, 2022

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

Let's wait @sokra answer

@koggdal
Copy link
Author

koggdal commented Jun 6, 2022

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! ❤️

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

No branches or pull requests

3 participants