mini-css-extract-plugin icon indicating copy to clipboard operation
mini-css-extract-plugin copied to clipboard

Feature: Disable Extracting Specified CSS Module from Chunks

Open lsycxyj opened this issue 6 years ago • 17 comments

This PR contains a:

  • [ ] bugfix
  • [x] new feature
  • [ ] code refactor
  • [x] test update
  • [ ] typo fix
  • [x] metadata update

Motivation / Use-Case

You may disable extracting css modules programmatically by passing a function.

const miniCssExtractPlugin = new MiniCssExtractPlugin({
  disableExtract({ module, isAsync }) {
    // Do whatever you want. Disable by content size for example:
    // return module.content.length < 10 * 1024;
    // Or disable extracting from all async chunks
    return isAsync;
  },
});

Breaking Changes

None

Additional Info

I found #348 doesn't work at all by now and it's not flexible enough, so I made another pull request based on recent master branch.

lsycxyj avatar Jul 20 '19 10:07 lsycxyj

Well... the lint problems may be a bit difficult to be fixed...

lsycxyj avatar Jul 20 '19 10:07 lsycxyj

What is use case? Why you need disable async css?, also i think we should solve this in other case, provide developers function shouldBeExtracted(options) { if (something) { return false } return true; }, it is allows solve many cases, not only async, fox example you can keep small css inside js and many other cases

alexander-akait avatar Jul 22 '19 10:07 alexander-akait

@evilebottnawi See this comment. I don't need to make extra an request to fetch the css file of its own async js chunk which it has been loaded.

lsycxyj avatar Jul 22 '19 15:07 lsycxyj

@lsycxyj please read my comment above, we can implement general solution instead only for one case

alexander-akait avatar Jul 22 '19 15:07 alexander-akait

@evilebottnawi For general solution, I think a "shouldBeExtracted" function for all modules would be much better. I can decide whether a css module depending on its information: whether it is an async module, content, file size, and more.

lsycxyj avatar Jul 22 '19 15:07 lsycxyj

@lsycxyj yes, feel free to update PR and implement this option :+1:

alexander-akait avatar Jul 22 '19 15:07 alexander-akait

@evilebottnawi I managed it. Now disableExtract option has been implemented.

lsycxyj avatar Jul 24 '19 14:07 lsycxyj

Look here https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/loader.js#L101, just skip module for keeping module in js

alexander-akait avatar Jul 24 '19 14:07 alexander-akait

@evilebottnawi Do you mean like my latest commit? (37ce619). I made less code difference. I'm afraid I don't quite understand how to skip module in https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/loader.js#L101

lsycxyj avatar Jul 24 '19 14:07 lsycxyj

@evilebottnawi Friendly ping

lsycxyj avatar Jul 25 '19 12:07 lsycxyj

Hi, @evilebottnawi, Is there anything else need to be improved? I suppose there can't be less code changes to implement this feature.

lsycxyj avatar Aug 09 '19 02:08 lsycxyj

In todo, a lot of issues, you can use own branch if need this asap

alexander-akait avatar Aug 09 '19 10:08 alexander-akait

Merge from latest master, fix bug and reopen

lsycxyj avatar Feb 04 '20 09:02 lsycxyj

Known issue: some variables will become "void 0" in some modules after enabling disableExtract in complicated cases, and I don't have any good idea that I can rebuild the modules correctly. Could you do me a favor, please? @evilebottnawi

lsycxyj avatar Feb 06 '20 09:02 lsycxyj

Codecov Report

Merging #432 (40af212) into master (b146549) will increase coverage by 1.13%. The diff coverage is 97.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
+ Coverage   88.55%   89.68%   +1.13%     
==========================================
  Files           5        5              
  Lines         428      475      +47     
  Branches       96      104       +8     
==========================================
+ Hits          379      426      +47     
  Misses         47       47              
  Partials        2        2              
Impacted Files Coverage Δ
src/index.js 89.49% <97.67%> (+1.64%) :arrow_up:
src/loader.js 90.90% <100.00%> (+1.43%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b146549...40af212. Read the comment docs.

codecov[bot] avatar Aug 06 '20 15:08 codecov[bot]

The functionally seem to be based on rebuilding modules. That's really problematic as it leads to inconsistencies and performance problems. Rebuilding modules should be avoided.

There is already the option to apply mini-css only to some modules via the rule system. I don't think this PR is needed anymore when the isAsync feature is removed.

@sokra Thank you for your review.

Actually, this feature is originally named "disableAsync". The main purpose of the feature is to find out async modules and disable the extraction programactically.

Then, @evilebottnawi gave me advice that to do it with a function like shouldBeExtracted, and it became "disableExtract".

I do understand it leads to some problems. However, I can't figure out any better way to implement the feature by now. I'm still looking for an alternative way.

lsycxyj avatar Aug 07 '20 02:08 lsycxyj

There is already the option to apply mini-css only to some modules via the rule system. I don't think this PR is needed anymore when the isAsync feature is removed.

Why this is unacceptable?

alexander-akait avatar Aug 07 '20 10:08 alexander-akait