mini-css-extract-plugin
mini-css-extract-plugin copied to clipboard
webpack-contrib
feat: hmr error recovery
This PR contains a:
- [ ] bugfix
- [X] new feature
- [ ] code refactor
- [X] test update
- [ ] typo fix
- [ ] metadata update
Motivation / Use-Case
This PR implements error-resilience/-recovery for HMR when dealing with failed CSS child compilations. (E.g. syntax errors in Less or Sass code.)
It enables failed compilations to continue to hot.accept future compilations, instead of requiring a full reload. This is a significant improvement to workflow, esp. for larger applications that can't deal well with reload and require manually recreating a certain application state.
It fixes #431
Breaking Changes
None, as far as I am aware. All existing test scenarios in the suite continue to pass.
Additional Info
The NetMatch/mini-css-extract-plugin-hmr-testcase repository contains a testcase that illustrates the problem; includes a method to switch to the patched fork and illustrate the improvements; and has a rundown of the how/what/why of changes.
A test update will probably still be required to verify the resilience. But I'm having some problems wrapping my head around how the test suite is set up. Some assistance there would be appreciated.
Not quite sure what's up here; I did sign the CLA but it remains pending as not signed?
Looks like it doesn't quite understand users with multiple email addresses. In particular commits originating from different e-mails tied to the same GitHub user.
I can (and have) signed using the e-mail under which commits were added, but then I can't sign anymore for the primary e-mail which GitHub uses to open the PR.
We need add tests, you can find hmr tests in test directory
I'll have a look for those and will try to figure out how to add a test for this.
@evilebottnawi
It looks like the tests for HMR.test.js are using mocking to perform only an isolated test of the hmrModuleReplacement runtime that is used by the hot loader code.
There don't seem to be any tests using complete HMR middleware and execution flow, so I don't think I can actually fit a test for these changes as it requires an actual HMR client to execute within.
(That's the exact bit I had problems with before when attempting to automate the repro-case, come to think of it.)
[EDIT] Having a look if I can add this to the manual test cases.
@evilebottnawi
Had another long look at how to make a test work.
A proper test for the full HMR flow (which in general is indeed currently not actually tested) would, I think, require something more or less like the following:
- A server implementation using
expresswithwebpack-dev-middlewareandwebpack-hot-middleware - A browser that can be instrumented, e.g. using
puppeteer - A basic testcase webpack project, like the one in the repository listed under 'additional info'
An overarching test harness is then needed to close the loop between server and client:
- Start the Express server (and the webpack compilation)
- Boot up Puppeteer and load the testcase
index.htmlpage - Start logging console output from Puppeteer.
- Wait for the "[HMR] Connected" message
- Change a source file on disk leading to a compilation error (e.g. introduce a syntax error into a Less or Sass file)
- Wait for the batch of "[HMR]" messages to come in from Puppeteer
- Match against expected output.
- etc. etc. following through the entire test scenario.
- shut down cleanly again
Does that sound acceptable?
I don't really like the idea of editing a file on disk for a test, but the alternative is creating a new in-memory file system and hooking that up as the input file system for Webpack. In which case such an in-memory file system would also need full support for watches and change monitoring, which afaik Webpack's memory-fs does not have. Seems definitely out-of-scope.
You don't need this stuff, just mock less-loader and throw error, no need e2e tests, unit tests are enough
You don't need this stuff, just mock
less-loaderand throw error, no need e2e tests, unit tests are enough
If you're only interested in the code output containing the expected hot-loader along with the error, yes; that test I can build for you easily. Just a dummy loader which always errors, chained onto the MiniCssExtractPlugin's loader.
@evilebottnawi
We need add tests, you can find hmr tests in test directory
Added a test which verifies that a module with compilation errors first emits hot bindings before throwing its error.
Of special note:
I had to introduce a convention on the TestCases.test.js that processes the /cases/ folder. That convention is that cases starting with a ~ are expected to throw and should not cause the unit test to fail. (As you might expect; the compiler still outputs an error when rendering the adapted HMR error recovery code. We don't - and probably shouldn't - cancel that out.)
[EDIT] Still looking at why this is failing on the build agents and not when run locally and what to do about it. Looks related to file location in the stack trace rendered into the error. Guess I can try to mock that.
[EDIT] And it's working now.
@evilebottnawi Please look on notes
Just a friendly ping to inform you that your noted remarks were processed. Not sure if Github is otherwise notifying you.
I'm having the exact same issue. Would be awesome to finish this PR. What still need to be done ?
What still need to be done ?
@evilebottnawi has to approve it and merge it. That's really all there's left to it, afaik.
There's a trivial merge-conflict in src/loader.js which is basically a function rename, introduced because this PR has been lingering for half a year. But they can still easily resolve it.
@rjgotten thank your for your message. I now @evilebottnawi is pretty busy working on many projects. So I'm not surprised that it's a bit long to merge. He probably don't have the time or maybe just forget this PR 😄
Hey @evilebottnawi, this is just a friendly ping ! It could be nice to finish this PR, she's getting old 😄 Thanks