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 stops updating after first error, reports as unaccepted module #431

Open
rjgotten opened this issue Jul 19, 2019 · 17 comments · May be fixed by #435
Open

HMR stops updating after first error, reports as unaccepted module #431

rjgotten opened this issue Jul 19, 2019 · 17 comments · May be fixed by #435

Comments

@rjgotten
Copy link

  • Operating System: Windows 10
  • Node Version: 10.x
  • NPM Version: 9.6.0
  • webpack Version: 4.35.3
  • mini-css-extract-plugin Version: 0.8.0

Expected Behavior

Given is a Less file styles.less, in which we accidentally introduce an error and then save.
HMR attempts an update and it shows the errored compilation.

After fixing the error, HMR should correctly load the fixed compiled CSS.

Actual Behavior

Instead of loading the fixed compiled CSS, HMR reports Ignored an update to unaccepted module ./styles.less and fails to update.

After having received a failed compilation, the module will continue to not accept hot updates until having manually reloaded the web page.

@rjgotten rjgotten changed the title HMR stops after first error HMR stops updating after first error, reports as unaccepted module Jul 19, 2019
@alexander-akait
Copy link
Member

Please create minimum reproducible test repo

@rjgotten
Copy link
Author

rjgotten commented Jul 19, 2019

Please create minimum reproducible test repo

You can't. This relies on interactive behavior reloading in developer mode after a live edit in the .less source file.

Or do you actually want just the base webpack.config.js with just less-loader and a module that imports the style sheet?

@alexander-akait
Copy link
Member

@rjgotten without reproducible test repo issue was closed because we can't help you with example

@rjgotten
Copy link
Author

Already figured out the problem.

MiniExtract use a pitching loader that starts a child compilation to grab a module produced by css-loader and then executes that module to obtain the CSS source.

If the .less file being compiled contains a syntax error it will cause that child compilation to fail.

Your loader will try/catch that, and will at that point propagate the error by passing it to its callback.
That will lead Webpack itself to produce a module that has a throw statement in it containing the build error.

The whole problem then becomes that the hot module replacement will dispose the old registration, but the new module source that is incoming to replacing it, only throws and doesn't contain any of the hot module registration code needed for a new module.hot.accept()


Here's what I would propose to resolve that problem:

Inside the try-catch that guards the execution of the module yielded by the child complation, you should
first check if hmr is enabled. If it is; then you should emit your own module source, mimicing the error module that Webpack would generate -- which is literally just throw new Error("<message>") -- but before the throw register a new instance of the hot loader.

That way, the hot module replacer will register for a future accept and the error becomes recoverable.

@alexander-akait
Copy link
Member

Can you don't ignore my message about reproducible test repo? A lot of developers use less and HMR and no problem was reported, so i want to ensure problem in plugin not in your configuration so i ask you create simple reproducible test repo

@rjgotten
Copy link
Author

I know the plugin is to blame.
Because I've just fixed it.

And yes; I'll work on a repro case.
Two actually. One which also demonstrates the fixed plugin.

That'll be during office hours next week.
I'm already an hour into overtime today.

@alexander-akait
Copy link
Member

alexander-akait commented Jul 19, 2019

Glad to hear, don't forget we need add test for a PR, so reproducible test repo help me solve this fastly with test

@rjgotten
Copy link
Author

rjgotten commented Jul 19, 2019

reproducible test repo

Ofcourse.
Note though, as mentioned before: triggering the problem requires editing CSS/Less/Sass code while the webpack project is up and running with HMR. So an automated test repo demonstrating it will be kind of hard to pull off.

So I hope you'll settle for a pre-baked project along with some manual instructions to trigger the problem.


On a sidenote, something I really feel I have to say, having now stepped into the source-code for MiniCssExtract and coming to grips with the techniques with which it 'does its magic':

The ideas behind how it works and how it integrates its hot-reloading is nothing short of genius.
Massive kudos for that.

@rjgotten
Copy link
Author

@evilebottnawi
I think I may have run across one particular reason why this particular problem with HMR halting at the first error, may be kind of rare to occur.

webpack-dev-server is hard-wired to reload when it comes across an unaccepted module, which is the code path that is triggered once the hot.accept binding is dropped and not re-established for a stylesheet module that doesn't compile.

webpack-hot-middleware integrating with Express (but also with more black-boxy stuff like Microsoft's Webpack support for ASP.NET Core) has automatic full-reloading as an option, and defaults it to disabled.
If you don't (or can't) enable it then you run into the particular problem described in this issue.

I have a repro-case of it all just about finished, along with a separate branch showing hardened HMR support in the loader part of MiniCssExtract, so that it can handle recovery even without a full reload.
And yes; it passes unit tests and linting.

(Kind of cool really. One of the boons of this plugin using a child compiler is that such a flow specifically allows you to intercept the child compilation's errors and handle them yourself in a non-fatal way.)


Ah... and that reminds me:
Apologies for allowing the (CR)LF linting issue to run out of hand earlier today.
I think tension ran just a tad too high there. 😄

@alexander-akait
Copy link
Member

@rjgotten don't worry, please create reproducible test repo, it is help to me catch problem place and think about solution, hmr is not easy, maybe problem in webpack-hot-middleware, maybe not, need look on full setup, thanks

@rjgotten
Copy link
Author

Looks like Github is currently experiencing some difficulties.
I have everything ready on my end, but it seems I'll have to wait until tomorrow to be able to push it.

Just letting you know.

@alexander-akait
Copy link
Member

@rjgotten yep, feel free to ping me

@rjgotten
Copy link
Author

@evilebottnawi

The testcase is available now as mini-css-extract-plugin-hmr-testcase.

The readme contains a few manual instructions. (Sorry, couldn't get that to work automated.)
It also contains some npm tasks to swap between the package version of mini-css-extract-plugin and the fork that I patched. And it contains a short rundown of what/how/why changes are applied to the forked plugin to make error-recovery/resilience work.

If you want I can also open a PR from the fork.

@alexander-akait
Copy link
Member

Thanks for reproducible test repo, feel free to send a PR anytime

@rjgotten rjgotten linked a pull request Jul 23, 2019 that will close this issue
6 tasks
@rjgotten
Copy link
Author

@evilebottnawi

PR is also up, but there seems to be an issue with the CLA-signing.
Bot gets confused when dealing with commits from separate emails which map to the same GitHub user, as far as I can see.

@alexander-akait
Copy link
Member

Sorry for big delay, I will look at this tomorrow

@rjgotten
Copy link
Author

Sorry for big delay, I will look at this tomorrow

No problem. Stuff like this happens.
Pretty sure that the like of Webpack 5 puts a lot of work on your plate.

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.

2 participants