-
Notifications
You must be signed in to change notification settings - Fork 296
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
Support reloading on failed update in webpack 5 #394
Conversation
@@ -65,12 +40,44 @@ module.exports = function(hash, moduleMap, options) { | |||
return null; | |||
} | |||
|
|||
var failedUpdate = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failedUpdate is false here then how it will get changed to true again when the update gets failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 62, 68, 79 in the new coe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood Thank you
@lukeapage Can you please publish your fork on npm.com? |
I suggest pointing at the GitHub address. Feel free to publish under your own user if you need it in npm. |
this will work too, thnaks |
Need rebase, I think we need test it too |
can confirm this fix works |
I am quite new to this repo so apologies in advance if I am off here, but isn't this just a workaround fix? There appears to be an underlying issue here in how webpack 5 is detecting changed modules, causing onUnaccepted to always be called (and therefore no page reload) as is happening in issues #390 and #398 . Isn't the advisable path here to go to the webpack repo instead and fix the changed module detection there, rather than workaround it with a forced reload when it (now always) fails? Again, sorry if I am misunderstanding. |
Any update on this? |
Is it a valid pr? when about to merge? |
I'm going to close this PR in favour of #397 as although this fix appears to work, no-one is clear why this change was necessary and what the implications of it are. I'm intended to treat that as the focus for future webpack 5 work by whoever has time or inclination to move it forwards. |
Sorry I didn’t document this well - it is a work around for a webpack bug - webpack/webpack#12408 It was a quick fix because I didn’t feel up to rewriting this plugin or making a big fix to webpack to fix it. Am happy staying on my fork for now - sorry if that means I’m not pushed to do a better fix. |
This PR contains a:
Motivation / Use-Case
Fixes #390