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

Better HMR story for css-modules :) #669

Closed
lydell opened this issue Jan 25, 2018 · 7 comments · Fixed by webpack-contrib/style-loader#298
Closed

Better HMR story for css-modules :) #669

lydell opened this issue Jan 25, 2018 · 7 comments · Fixed by webpack-contrib/style-loader#298

Comments

@lydell
Copy link

lydell commented Jan 25, 2018

Do you want to request a feature or report a bug?
feature

What is the current behavior?
I have enabled CSS HMR. It’s awesome!

If I enable css-modules, saving a CSS file now results in the whole page being reloaded. As mentioned #139 this is correct: JS files imports class names from CSS file, and those could have changed.

I think we can do better though :)

What is the expected behavior?

webpack should only reload the full page if it really has to.

Most of the time, I fiddle with values for width, padding and margin. Only changing those should not need a full reload, right?

  • Only adding, removing or changing properties should not cause a full reload.
  • Adding, removing or changing rules/selectors should cause a full reload.

If something like the above isn’t possible, I would suggest an “I know what I’m doing and accept the downsides” option, that tells webpack to never do a full reload when a CSS file changes, leaving it up to the user to manually reload when they change class names.

If this is a feature request, what is motivation or use case for changing the behavior?

CSS HMR is such a productivity boost! It’s so sad to see it go when enabling css-modules.

Please mention other relevant information such as your webpack version, Node.js version and Operating System.

webpack version: 3.10.0
Node.js version: 8.9.4
Operating System: Ubuntu 16.04

@alexander-akait
Copy link
Member

@lydell seems you have something wrong in configuration, css-loader don't reload page when you change css/less/scss/etc. Can you provide configuration or minimum reproducible test repo?

@lydell
Copy link
Author

lydell commented Jan 25, 2018

Sorry, I was 100% sure that this was the way things worked from all the issues I’ve been reading through! Might totally be a problem with my configuration. I’ll come back with a test repo.

@lydell
Copy link
Author

lydell commented Jan 25, 2018

@evilebottnawi Here is a minimal reproduction repo: https://github.com/lydell/css-hmr-bug

@alexander-akait
Copy link
Member

@lydell thanks!

@lydell
Copy link
Author

lydell commented Jan 25, 2018

I looked at the compiled code. As far as I can tell, HMR is completely disabled when using css-modules.

/******/ ({

/***/ "./index.css":
/***/ (function(module, exports, __webpack_require__) {

// style-loader: Adds some css to the DOM by adding a <style> tag

// load the styles
var content = __webpack_require__("./node_modules/css-loader/index.js?{\"modules\":true}!./index.css");
if(typeof content === 'string') content = [[module.i, content, '']];
// Prepare cssTransformation
var transform;

var options = {"hmr":true}
options.transform = transform
// add the styles to the DOM
var update = __webpack_require__("./node_modules/style-loader/lib/addStyles.js")(content, options);
if(content.locals) module.exports = content.locals;
// Hot Module Replacement
if(true) {
	// When the styles change, update the <style> tags
	if(!content.locals) {
		module.hot.accept("./node_modules/css-loader/index.js?{\"modules\":true}!./index.css", function() {
			var newContent = __webpack_require__("./node_modules/css-loader/index.js?{\"modules\":true}!./index.css");
			if(typeof newContent === 'string') newContent = [[module.i, newContent, '']];
			update(newContent);
		});
	}
	// When the module is disposed, remove the <style> tags
	module.hot.dispose(function() { update(); });
}

/***/ }),

The interesting part is:

if(!content.locals) {
  module.hot.accept("./node_modules/css-loader/index.js?{\"modules\":true}!./index.css", function() {

I checked in the debugger, and content.locals is an object like this when using css-modules (it is what you get from import styles from "./index.css"):

{
  test: "_1vEBNtLiWWztNT6HBkQqKm"
}

@evilebottnawi Do you know which repo that HMR code comes from? style-loader?

Do you have any idea on how to implement my proposal? I'm thinking that I must compare old and new content.locals and conditionally accept the hot update somehow.

@lydell
Copy link
Author

lydell commented Jan 26, 2018

Never mind, I think I figured it all out. PR coming soon.

lydell added a commit to lydell/style-loader that referenced this issue Jan 26, 2018
lydell added a commit to lydell/style-loader that referenced this issue Jan 26, 2018
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.
@lydell
Copy link
Author

lydell commented Jan 26, 2018

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

Successfully merging a pull request may close this issue.

2 participants