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(index): enable HMR in case locals (css-modules) are unchanged #298

Merged
merged 6 commits into from
Jan 26, 2018

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Jan 26, 2018

What kind of change does this PR introduce?
fix/improvement

Did you add tests for your changes?
No, I couldn’t find that this type of thing has tests already. Nudge me in the right direction and I’ll add some!

I did make this test repo though to show that it works: https://github.com/lydell/css-hmr-bug/tree/fix

(The master branch shows the problem, while the fix branch shows the fix.)

If relevant, did you update the README?
Not relevant.

Summary
Fixes webpack-contrib/css-loader#669.

Previously, HMR was disabled if css-modules was enabled (in css-loader) and at least one class was present in the file. This was because JS files depend on the classes, and as such have to be reloaded when the CSS changes in case the classes have changed.

Now, HMR is used if no classes change. Othewise the whole page is reloaded like before.

CSS HMR is awesome! css-modules are great, too. Now you can have both! 🎉

Does this PR introduce a breaking change?

No breaking changes!

Fixes webpack-contrib/css-loader#669.

Previously, HMR was disabled if css-modules was enabled (in css-loader)
and at least one class was present in the file. This was because JS
files depend on the classes, and as such have to be reloaded when the
CSS changes in case the classes have changed.

Now, HMR _is_ used if no classes change. Othewise the whole page is
reloaded like before.
@jsf-clabot
Copy link

jsf-clabot commented Jan 26, 2018

CLA assistant check
All committers have signed the CLA.

@@ -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.

index.js Outdated
" });",
" }",
" }",
" 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.

@lydell
Copy link
Contributor Author

lydell commented Jan 26, 2018

Travis failed in Node.js 4.3 because of an unrelated issue?

$ yarn run test
error Couldn't find the binary yarn run test

The tests pass locally.

index.js Outdated
" }",
" } 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

@lydell
Copy link
Contributor Author

lydell commented Jan 26, 2018

I’ve updated https://github.com/lydell/css-hmr-bug/tree/fix now to point to the correct git SHA for my style-loader fork, so it should work now.

@michael-ciniawsky michael-ciniawsky changed the title fix: Allow css-modules HMR when classes are unchanged fix(index): allow HMR in case locals are unchanged (css-modules) Jan 26, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

This needs to be tested for the next branch aswell, where locals are named ESM Exports instead of an {Object} and if the approach taken here is upgradable

index.js Outdated
" 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

index.js Outdated
" } 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

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.

index.js Outdated
" if(oldKeys.length !== newKeys.length) {",
" 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]
})

index.js Outdated
" });",
" }",
" }",
" if(localsSame) {",
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('...')

index.js Outdated
" }",
" } 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
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

@lydell
Copy link
Contributor Author

lydell commented Jan 26, 2018

This needs to be tested for the next branch aswell, where locals are named ESM Exports instead of an {Object} and if the approach taken here is upgradable

I know nothing about the next branch – what do I need to do more specifically?

@lydell
Copy link
Contributor Author

lydell commented Jan 26, 2018

Does this PR introduce a breaking change?

Hmm, I just realized Object.keys and Array.prototype.every aren’t supported in IE8 – is that a problem? Which browsers are supported?

@michael-ciniawsky
Copy link
Member

Yeah that's probably a major then and would need be developed against the next branch

@michael-ciniawsky
Copy link
Member

What's the minimum IE version with support for Object.keys && Array.every ?

@alexander-akait
Copy link
Member

@michael-ciniawsky we can accept this as bug here and release patch version, then do PR with this feature to next branch, after this will be resolved and merge

@alexander-akait
Copy link
Member

alexander-akait commented Jan 26, 2018

Also I want to note that we should support some time two versions (0.x.x and next) because, not everyone can change the version quickly and without tests and regressions 😄

@michael-ciniawsky
Copy link
Member

Not if this breaks the current browsers supported, which we need to figure out/ agreed upon a minimum version supported (if never specified somewhere)

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Jan 26, 2018

Agreed but this more a feature/refactor with eventual breaking changes for master then a (critical) bug

webpack-contrib/css-loader#139 (comment)

@lydell
Copy link
Contributor Author

lydell commented Jan 26, 2018

IE9 is the lowest. I agree with @evilebottnawi , so I’ll get rid of Object.keys and Array.prototype.every so that we can merge this into master.

@alexander-akait
Copy link
Member

@michael-ciniawsky it is looks as bug, because we don't have any note(s) about this don't works and i think it should be work as expected and says in docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better HMR story for css-modules :)
5 participants