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.

Sign up for GitHub

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

Jump to bottom

fix(index): enable HMR in case locals (css-modules) are unchanged #298

Merged
merged 6 commits into from
Jan 26, 2018
Merged

fix(index): enable HMR in case locals (css-modules) are unchanged #298

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d96c43
fix: Allow css-modules HMR when classes are unchanged
lydell Jan 26, 2018
3bb5ae5
Shorten some code
lydell Jan 26, 2018
19ba925
ES3 (IE8) support
lydell Jan 26, 2018
1007d40
Use worse variable names
lydell Jan 26, 2018
b516fc0
Add missing semicolon
lydell Jan 26, 2018
0d52dee
Add another missing semicolon
lydell Jan 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,22 @@ module.exports.pitch = function (request) {
"// Hot Module Replacement",
"if(module.hot) {",
" // When the styles change, update the <style> tags",
" if(!content.locals) {",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I removed this check, I dedented the code so the diff looks a little bit larger than it is.

" module.hot.accept(" + loaderUtils.stringifyRequest(this, "!!" + request) + ", function() {",
" var newContent = require(" + loaderUtils.stringifyRequest(this, "!!" + request) + ");",
" if(typeof newContent === 'string') newContent = [[module.id, newContent, '']];",
" update(newContent);",
" });",
" }",
" module.hot.accept(" + loaderUtils.stringifyRequest(this, "!!" + request) + ", function() {",
" var newContent = require(" + loaderUtils.stringifyRequest(this, "!!" + request) + ");",
" if(typeof newContent === 'string') newContent = [[module.id, newContent, '']];",
" var locals = (function(a, b) {",
" var key, idx = 0;",
" for(key in a) {",
" if(!b || a[key] !== b[key]) return false;",
" idx++;",
" }",
" for(key in b) idx--;",
" return idx === 0;",
" }(content.locals, newContent.locals));",
" // This error is caught and not shown and causes a full reload.",
" if(!locals) throw new Error('Aborting CSS HMR due to changed css-modules locals.');",
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove comment from source code (move as comment before this line), it is increase bundle in dev mode, Good work, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, but wouldn't that be inconsistent with all the other comments that are part of the output source code?

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, but the fact is that this code needs a few refactorings anyways in the future :) @evilebottnawi suggestion is a good idea and I will do it for all the source comments before cutting a release now. @evilebottnawi anything else of concern otherwise I would land this now and take care of the rest ?

Copy link
Member

Choose a reason for hiding this comment

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

@lydell oh yes, let's fix it in next PR

Copy link
Member

Choose a reason for hiding this comment

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

@michael-ciniawsky merge as is and fix it next PR

" update(newContent);",
" });",
" // When the module is disposed, remove the <style> tags",
" module.hot.dispose(function() { update(); });",
"}"
Expand Down