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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

latin-1
Copy link

@latin-1 latin-1 commented Dec 28, 2022

This PR contains a:

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

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

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid it will reload files very often too, especial for nested @import

@latin-1
Copy link
Author

latin-1 commented Feb 5, 2023

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

@latin-1
Copy link
Author

latin-1 commented Feb 5, 2023

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

latin-1 commented Feb 6, 2023

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

latin-1 commented Feb 20, 2023

Test cases updated.

@alexander-akait
Copy link
Member

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

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

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

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

❗ 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
@@            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 Coverage Δ
src/loader.js 89.01% <ø> (ø)
src/hmr/hotModuleReplacement.js 91.86% <100.00%> (+0.34%) ⬆️

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

@latin-1
Copy link
Author

latin-1 commented May 5, 2023

ping @alexander-akait

@alexander-akait
Copy link
Member

@latin-1 Sorry for delay, I am working on built-in CSS supports in webpack webpack/webpack#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
Copy link
Member

Feel free to ask questions

@latin-1
Copy link
Author

latin-1 commented May 5, 2023

@alexander-akait got it. thanks for your explanation

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

Successfully merging this pull request may close these issues.

Adding new css import doesn't result in hot style update
2 participants