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

HMR does not work as of 1.0.0 #422

Closed
phaistonian opened this issue Aug 9, 2019 · 34 comments · Fixed by #466
Closed

HMR does not work as of 1.0.0 #422

phaistonian opened this issue Aug 9, 2019 · 34 comments · Fixed by #466

Comments

@phaistonian
Copy link

  • Operating System: OSX
  • Node Version: Latest
  • NPM Version: Latest
  • webpack Version: Latest
  • style-loader Version: 1.0.0

Expected Behavior

HTR to work as before 1.0.0

Actual Behavior

Ignored an update to unaccepted module kind of errors are thrown and full-reload takes place whenever a change is made in content passed to style-loader.

How Do We Reproduce?

Not sure - I guess try changing css content using the latest version of style-loader.

@alexander-akait
Copy link
Member

alexander-akait commented Aug 9, 2019

yes, because you use css modules and previously behavior break many other cases, it was invalid solution and we revert this

From changelog:

restore original hot reloading behaviour for locals (#419) (f026429)

We implement new HMR api for webpack@5 and it will be possible, right now it is won't fix

@alexander-akait
Copy link
Member

@phaistonian anyway please create reproducible test repo to ensure it is situation described above

@phaistonian
Copy link
Author

So, HMR will be working as it should when webpack 5 is released and until then we should revert to a version < 1.0, yes?

@alexander-akait
Copy link
Member

alexander-akait commented Aug 9, 2019

@phaistonian hmr works fine if you don't use css modules, if you use css modules you will get full reload with style-loader, if you revert to old version you get invalid hmr behavior what potentially break app in some cases in dev mode, anyway please create minimum reproducible test repo

@phaistonian
Copy link
Author

phaistonian commented Aug 9, 2019

@evilebottnawi Will try to - once I get some free time.

Thank you.

ps: I am not using CSS modules, the setting in css-loader is the default, i.e false.

@alexander-akait
Copy link
Member

@phaistonian should work, maybe bug, wait reproducible test repo and help you

@laino
Copy link

laino commented Aug 16, 2019

It's also broken for me. Just using style-loader, css-loader, sass-loader. With default settings.

@alexander-akait
Copy link
Member

PLEASE create minimum reproducible test repo, message like It's also broken for me is not help

@Yegorich555
Copy link

@phaistonian hmr works fine if you don't use css modules, if you use css modules you will get full reload with style-loader, if you revert to old version you get invalid hmr behavior what potentially break app in some cases in dev mode, anyway please create minimum reproducible test repo

I agree. Previous version of style-loader works fine even with css-modules. HMR in the version 1.0.0 doesn't work as expected only with css-modules. I'm frustrated of this. Css-modules mustHave for me and I can't work without HMR at all. And I've never had potentially breaking app in some cases in dev mode with previous versions. Can it be fixed for the latest version?
The repo for reproducing you can find here: https://github.com/Yegorich555/FromScratch_React/tree/styleloader-update

@alexander-akait
Copy link
Member

@Yegorich555 previously version style-loader didn’t work correctly, even if everything seemed to be right for you, technically it was done incorrectly and led to problems in many places, it can be fixed only introducing new hot api, and in theory can be done for webpack@5. What is problem? Refreshing is very fast

@Yegorich555
Copy link

Yegorich555 commented Nov 15, 2019

The main problem is not in the speed of refreshing but in refreshing in general. For example I'm developing some view or component which is deeply nested and with refreshing everything is lost. And I need to go to the exact view by clicking 4 times (on some buttons) and filling forms and submitting 2 forms, for example. You can imagine how much time I spend with such refreshes (during the css bug-fixing for example).

So I got your point on the latest update and it's fine that it works in the right way, but hot-reload with css-modules is not only convenient but it decreases time of developing also. In this case I can't use the latest update. Time of developing is number 1 for me.

Could you add some points in Readme and Changelog that can clarify breaking changes for css-modules hot reload?

@alexander-akait
Copy link
Member

@Yegorich555 yep, in near future, maybe we can implement new hot API for webpack@5 and do backport for webpack@4 it is allow to improve this, but unfortunately i don't have enough time and it can take a lot of time

@phaistonian
Copy link
Author

So, to recap:

  • We can't have hot-reloading with 1.0.0
  • We might have it again when Webpack 5 is a released via a new API

yes?:)

@alexander-akait
Copy link
Member

@phaistonian yes, or we can implement new api in webpack@4 and implement this in 1.0.0

@phaistonian
Copy link
Author

@evilebottnawi I am not sure it makes much sense developing (i.e in local environment) and not having HMR on css. It's definetely a deal-breaker in our cases and that's why we downgraded to the previous version across all our repos.

My suggestion is to do so and support webpack 5 later - if more work is needed.

@alexander-akait
Copy link
Member

@phaistonian I will have a discussion with sokra about it, maybe we can return this faster than you think, in my todo

@chybisov
Copy link

chybisov commented Jan 26, 2020

@evilebottnawi do you have any updates on this? Thank you!

@alexander-akait
Copy link
Member

@chybisov right now no, sorry, we have new HMR api for webpack@5, maybe we should start to backport it for webpack@4

@egucciar
Copy link

As we're using a new api introduced in 1.x, we unfortunately cannot downgrade style-loader to get HMR. Any advice would be greatly appreciated to get this.

@wind4gis
Copy link

wind4gis commented Mar 8, 2020

I can't use style-loader hot update, it sends an empty array by websocket while I modify the scss..and because the empty array, the page won't refresh..is there anything we can do or only the way downgrading to v0.23.1 ??
image
image

@wind4gis
Copy link

wind4gis commented Mar 8, 2020

emm...I use the "@hot-loader/react-dom": "^16.12.0" and "react-hot-loader": "^4.12.19", and I downgrade to v0.23.1...but hmr still not work, whatever I use normal css or css module...

@alexander-akait
Copy link
Member

I think we can implement it now https://github.com/webpack/webpack/releases/tag/v4.43.0

@chybisov
Copy link

@evilebottnawi great news! Will you be able to implement this in the near future? Thank you!

@Timer
Copy link

Timer commented Apr 21, 2020

I believe something along these lines would fix the behavior for webpack 4.43+:

diff --git a/src/index.js b/src/index.js
index 5907d16..ca87820 100644
--- a/src/index.js
+++ b/src/index.js
@@ -187,7 +187,7 @@ ${esModule ? 'export default' : 'module.exports ='} exported;`;
       const hmrCode = this.hot
         ? `
 if (module.hot) {
-  if (!content.locals) {
+  if (module.hot.invalidate || !content.locals) {
     module.hot.accept(
       ${loaderUtils.stringifyRequest(this, `!!${request}`)},
       function () {
@@ -205,6 +205,30 @@ if (module.hot) {
                 newContent = [[module.id, newContent, '']];
               }
 
+              if (content.locals) {
+                if (!newContent.locals) {
+                  module.hot.invalidate();
+                  return;
+                }
+
+                var oldKeys = Object.keys(content.locals),
+                  newKeys = Object.keys(newContent.locals);
+                if (oldKeys.length !== newKeys.length) {
+                  module.hot.invalidate();
+                  return;
+                }
+
+                for (var idx = 0; idx < oldKeys.length; idx++) {
+                  var key = oldKeys[idx];
+                  if (content.locals[key] === newContent.locals[key]) {
+                    continue;
+                  }
+
+                  module.hot.invalidate();
+                  return;
+                }
+              }
+
               update(newContent);`
         }
       }

@alexander-akait
Copy link
Member

I will implement it today

@alexander-akait
Copy link
Member

Release will be tomorrow, need write a lot of tests

@alexander-akait
Copy link
Member

Do not forget that change locals (for example change @value v-color: red; to @value v-color: blue;) still require full page reloading, because we don't know your logic in component(s), for react you can use react-hot-loader

@phaistonian
Copy link
Author

It seems that the issue is still there - for me.

Changing a .styl file still triggers a full-page reload whereas this was not the case in version < 1.0.0.

@phaistonian
Copy link
Author

@evilebottnawi As noted in my previous comment, I had to downgrade to 0.23.1 - again.

It seems the newest version did not the trick.

@chybisov
Copy link

chybisov commented Apr 27, 2020

@phaistonian for css and scss files it works great.

@phaistonian
Copy link
Author

@chybisov I am quite sure this has nothing to do with stylus (and stylus loader) because the input to style loader is always CSS (string).

@alexander-akait
Copy link
Member

@phaistonian Please create reproducible test repo

@phaistonian
Copy link
Author

@evilebottnawi Busy times down here - so it might take a while.

Note that the error, this time, is not Ignored an update to unaccepted module but that webpack does not know how to handle an update on a module or on one of its parents.

@alexander-akait
Copy link
Member

alexander-akait commented Apr 28, 2020

@phaistonian Sorry without reproducible test repo I can't help, you can provide webpack config

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

Successfully merging a pull request may close this issue.

8 participants