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 1 commit
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
33 changes: 27 additions & 6 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,34 @@ 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, '']];",
" module.hot.accept(" + loaderUtils.stringifyRequest(this, "!!" + request) + ", function() {",
" var newContent = require(" + loaderUtils.stringifyRequest(this, "!!" + request) + ");",
" if(typeof newContent === 'string') newContent = [[module.id, newContent, '']];",
" var localsSame = true;",
Copy link
Member

Choose a reason for hiding this comment

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

Is the name locals available ?

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 guess. But isn’t that a worse name? localsSame answers the question “Are the locals the same?” while locals sounds like an object or array of local somethings.

Copy link
Member

Choose a reason for hiding this comment

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

The logic checks for identity of the locals (it's in the code), so if the name locals isn't already declared, locals is shorter and is imho clearer further below

// Do we have (new) locals => no HMR
if (!locals) throw new Error('..')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your comment is wrong.

  • Do we have more locals than before? => no HMR
  • Do we have fewer locals than before? => no HMR
  • Do we have the same number of locals as before, but some changed? => no HMR
  • Otherwise HMR

In other words:

  • Are the locals the same? HMR
  • Otherwise no HMR

" if(!content.locals && newContent.locals) {",
" localsSame = false;",
" } else if(content.locals && !newContent.locals) {",
" localsSame = false;",
" } else if(content.locals && newContent.locals) {",
" var oldKeys = Object.keys(content.locals);",
" var newKeys = Object.keys(newContent.locals);",
" if(oldKeys.length !== newKeys.length) {",
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. 

const length = Object.keys(content.locals).length - Object.keys(newContent.locals).length

if (length === 0) {
   ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? I find this harder to understand. And I use oldKeys a couple of lines down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const length – the length of what? There’s nothing with length 0 around.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, sry I meant

if (!length) {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit.

" localsSame = false;",
" } else {",
" localsSame = oldKeys.every(function(oldKey) {",
Copy link
Member

Choose a reason for hiding this comment

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

Object.keys(content.locals).every(function (key) {
   return content.locals[key] === newContent.locals[key]
})

" var oldValue = content.locals[oldKey];",
" var newValue = newContent.locals[oldKey];",
" return oldValue === newValue;",
" });",
" }",
" }",
" if(localsSame) {",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comparison between old and new locals is the gist of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

if (!locals) throw new Error('...')

" update(newContent);",
" });",
" }",
" } else {",
" // This error is caught and not shown and causes a full reload.",
" throw new Error('Aborting CSS HMR due to changed css-modules 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.

I looked through https://webpack.js.org/api/hot-module-replacement/ and tried to read the HMR source code in the webpack repo, and could not find any other way of aborting an accept other than throwing an error. It seems to work flawlessly, though!

Copy link
Member

Choose a reason for hiding this comment

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

I will take a look into this, I think it's fine

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