webpack-contrib / mini-css-extract-plugin

Lightweight CSS extraction plugin
MIT License
4.67k stars 388 forks source link

fix: perform a HMR update after inserting a new CSS module #1006

Open latin-1 opened 1 year ago

latin-1 commented 1 year ago

This PR contains a:

Motivation / Use-Case

Fixes #706

Breaking Changes

None

Additional Info

This PR is based on #726

Repo for testing: https://github.com/latin-1/mini-css-extract-plugin-706-repro

latin-1 commented 1 year ago

@alexander-akait I updated the implementation. It should no longer reload multiple times for nested @imports.

latin-1 commented 1 year ago
           style-a.css
         /             \
index.js                 shared.css
         \             /
           style-b.css
module.hot.dispose(cssReload); // 1. dispose callback, current behavior
module.hot.accept(undefined, cssReload); // 2. accept callback, current behavior
if (module.hot.status() !== "idle") {
  cssReload(); // 3. execute, newly introduced
}

Case 1: Update style-a.css

  1. style-a.css: disposing (1. dispose callback, current behavior)
  2. style-a.css: applying (3. execute, newly introduced)

Case 2: Update style-b.css

ditto

Case 3: Update shared.css

  1. style-b.css: disposing (1. dispose callback, current behavior)
  2. style-a.css: disposing (1. dispose callback, current behavior)
  3. style-a.css: applying (3. execute, newly introduced)
  4. style-b.css: applying (3. execute, newly introduced)

For the case 3, we've already reloaded multiple times. We may consider changing the key to the script url.

latin-1 commented 1 year ago

Because disposing and applying running in different contexts, I don't believe there is a better way than tracking in a global state.

latin-1 commented 1 year ago

Test cases updated.

alexander-akait commented 1 year ago

Hello, sorry for delay, I will look at this today

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 :tada:

Comparison is base (d2516c4) 90.33% compared to head (77ec64b) 90.39%.

:exclamation: Current head 77ec64b differs from pull request most recent head 32659cd. Consider uploading reports for the commit 32659cd to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1006 +/- ## ========================================== + Coverage 90.33% 90.39% +0.05% ========================================== Files 5 5 Lines 838 843 +5 Branches 229 230 +1 ========================================== + Hits 757 762 +5 Misses 71 71 Partials 10 10 ``` | [Impacted Files](https://app.codecov.io/gh/webpack-contrib/mini-css-extract-plugin/pull/1006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webpack-contrib) | Coverage Δ | | |---|---|---| | [src/loader.js](https://app.codecov.io/gh/webpack-contrib/mini-css-extract-plugin/pull/1006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webpack-contrib#diff-c3JjL2xvYWRlci5qcw==) | `89.01% <ø> (ø)` | | | [src/hmr/hotModuleReplacement.js](https://app.codecov.io/gh/webpack-contrib/mini-css-extract-plugin/pull/1006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=webpack-contrib#diff-c3JjL2htci9ob3RNb2R1bGVSZXBsYWNlbWVudC5qcw==) | `91.86% <100.00%> (+0.34%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

latin-1 commented 1 year ago

ping @alexander-akait

alexander-akait commented 1 year ago

@latin-1 Sorry for delay, I am working on built-in CSS supports in webpack https://github.com/webpack/webpack/issues/14893, it works good for pure CSS (except public path for assets modules, but I want to fix it soon), and we can just copy/paste logic for HMR from https://github.com/webpack/webpack/blob/main/lib/css/CssLoadingRuntimeModule.js

alexander-akait commented 1 year ago

Feel free to ask questions

latin-1 commented 1 year ago

@alexander-akait got it. thanks for your explanation