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: avoid reloading all csses when hot load #1090
fix: avoid reloading all csses when hot load #1090
Conversation
cfd1040
to
d8230d3
Compare
? "" | ||
: "module.hot.accept(undefined, cssReload);"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self accept is enough, it will update when dispose: https://webpack.js.org/api/hot-module-replacement/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a test case, please add
ea2c589
to
bff1a98
Compare
test cases updated |
bff1a98
to
3695ac6
Compare
src/loader.js
Outdated
// ${Date.now()} | ||
var cssReload = require(${stringifyRequest( | ||
const cssReload = require(${stringifyRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use const, it will be in the browser and break old browsers
? "" | ||
: "module.hot.accept(undefined, cssReload);"; | ||
|
||
const localsJsonString = JSON.stringify(JSON.stringify(context.locals)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why double JSON.stringify
? Just typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note the interpolation:
`const localsJsonString = ${localsJsonString};`
string comparation is faster
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1090 +/- ##
==========================================
+ Coverage 90.61% 92.00% +1.38%
==========================================
Files 5 5
Lines 895 900 +5
Branches 255 258 +3
==========================================
+ Hits 811 828 +17
+ Misses 74 67 -7
+ Partials 10 5 -5 ☔ View full report in Codecov by Sentry. |
test/cases/hmr/expected/main.js
Outdated
module.hot.dispose(function(data) { | ||
data.value = localsJsonString; | ||
cssReload(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a special test directory for HMR, will be great to add a test case there, I know your code works fine, but we should ensure about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean? I have added cases inside cases folder (hmr, hmr-locals)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay, just put a test case here https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/HMR.test.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay, just put a test case here https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/HMR.test.js
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do small improvements and I am fine with it
For fixing tests for OLD API just use |
Thank you for your PR |
3695ac6
to
e9bf629
Compare
passed |
e9bf629
to
977c990
Compare
@yiminghe We have:
As you can see it is an absolute path, let's use it from here https://github.com/webpack-contrib/css-loader/blob/master/src/utils.js#L15 (maybe we already have the same util here) |
977c990
to
9bfc65e
Compare
updated |
1a56673
into
webpack-contrib:master
This PR contains a:
Motivation / Use-Case
Breaking Changes
Additional Info
Fixes #692