feat: hmr error recovery #435

rjgotten · 2019-07-23T10:10:48Z

This PR contains a:

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.

jsf-clabot · 2019-07-23T10:10:51Z

All committers have signed the CLA.

rjgotten · 2019-07-23T10:17:13Z

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.

alexander-akait · 2019-07-23T10:39:23Z

src/loader.js

@@ -152,6 +166,10 @@ export function pitch(request) {
      this.addContextDependency(dep);
    }, this);

+    if (compilation.errors.length > 0) {
+      return callback(compilation.errors[0]);
+    }


For catching errors, right?

Yes. It was moved down passed the dependencies, to ensure those dependenices are always tracked.

Flipping the error state into a success state on the parent compilation, means that it will also update its registered dependencies. (If the parent compilation would fail, it would retain the previous set.)

If we'd drop out of the child compilation because of an error before the dependencies of the child compilation are moved into the parent, then we would 'lose' those dependencies, including any change watches on them. This way, we won't.

Would there be a case where multiple errors need to be shown in the errors array?

@ScriptedAlchemy
Not sure actually. The current version of the plugin only reports the first child compilation error as well.

alexander-akait

We need add tests, you can find hmr tests in test directory

rjgotten · 2019-07-23T11:00:39Z

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.

rjgotten · 2019-07-23T13:17:16Z

@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 express with webpack-dev-middleware and webpack-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.html page
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.

alexander-akait · 2019-07-23T13:20:35Z

You don't need this stuff, just mock less-loader and throw error, no need e2e tests, unit tests are enough

rjgotten · 2019-07-23T13:31:47Z

You don't need this stuff, just mock less-loader and 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.

alexander-akait · 2019-07-23T13:33:45Z

@rjgotten this enough

rjgotten · 2019-07-24T07:33:20Z

@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.

Needed to erase stack trace location, which varies by system

alexander-akait

Please look on notes

alexander-akait · 2019-07-24T10:28:24Z

test/cases/~hmr-resilience/error-loader.js

@@ -0,0 +1,4 @@
+module.exports = function loader() {


Don't use ~ in filename

Any other suggestion then for a moniker to use to indicate tests that are expected to fail?

Testcases.test.js works by enumerating directory names and running all the cases, then bailing if one of them has a compilation error. But here we have a testcase which is expected to have a compilation error. First of its kind.

@rjgotten just use fail word in test name

Adjusted the test pattern in TestCases.test.js to /-fail$/ i.e. any test suffixed with -fail is expected to error. And in accordance renamed the testcase to hmr-resilience-fail, getting rid of the ~.

alexander-akait · 2019-07-24T10:28:59Z

test/cases/~hmr-resilience/webpack.config.js

+  output: {
+    filename: '[name].js',
+  },
+  optimization: {


Why it is necessary for test?

output.filename is a leftover. I'll remove it.

The optimization settings are there to force everything but the compiled module itself into a separate chunk (which is then also not emitted).
This limits the surface of the test, so we don't end up comparing code for e.g. the hot reloader client itself. (Which may actually be subject to change and cause false-negative failed unit tests if it is ever updated.)

Removed the filename leftover, and left a comment explaining why the optimization chunk settings are being used.

alexander-akait · 2019-07-24T10:29:13Z

test/cases/~hmr-resilience/expected/check.js

+      // 1479427200000
+      var cssReload = __webpack_require__("../../../src/hmr/hotModuleReplacement.js")(module.i, {"hmr":true,"locals":false});
+      module.hot.dispose(cssReload);
+      module.hot.accept(function(){});


A little more context please:
why what ?

Why we need this module.hot.accept(function(){});? here, please add comment to source doe, because this line is misleading for other developers

Right. Will add a small description in the loader.js where this particular line of code is emitted.

- removes obsolete output filename from testcase; - renames testcase to remove ~ from directory name; - adds comment explaining use of optimization chunks in testcase; - adds comment explaining self-accept with empty error handler

rjgotten · 2019-07-30T09:18:03Z

@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.

alexander-akait · 2019-07-30T09:19:44Z

in todo list

ron23 · 2020-05-13T02:40:52Z

Hi, any progress on this?

jimblue · 2020-05-28T20:17:44Z

I'm having the exact same issue.
Would be awesome to finish this PR.
What still need to be done ?

rjgotten · 2020-05-29T06:43:56Z

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.

jimblue · 2020-05-29T10:59:12Z

@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 😄

jimblue · 2020-06-02T23:17:14Z

Hey @evilebottnawi, this is just a friendly ping !
It could be nice to finish this PR, she's getting old 😄
Thanks

feat: hmr error recovery

17acb10

alexander-akait reviewed Jul 23, 2019

View reviewed changes

alexander-akait requested changes Jul 23, 2019

View reviewed changes

feat: hmr error recovery test

607532d

rjgotten added 2 commits July 24, 2019 11:00

fix: amends hmr error recovery tests

ad42401

Needed to erase stack trace location, which varies by system

fix: remove unused testcase file

ed9f295

alexander-akait requested changes Jul 24, 2019

View reviewed changes

fix: hmr error recovery review remarks

380f822

- removes obsolete output filename from testcase; - renames testcase to remove ~ from directory name; - adds comment explaining use of optimization chunks in testcase; - adds comment explaining self-accept with empty error handler

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hmr error recovery #435

feat: hmr error recovery #435

rjgotten commented Jul 23, 2019 •

edited

jsf-clabot commented Jul 23, 2019 •

edited

rjgotten commented Jul 23, 2019 •

edited

alexander-akait Jul 23, 2019

rjgotten Jul 23, 2019 •

edited

ScriptedAlchemy Jul 24, 2019

rjgotten Jul 24, 2019

alexander-akait left a comment

rjgotten commented Jul 23, 2019 •

edited

rjgotten commented Jul 23, 2019 •

edited

alexander-akait commented Jul 23, 2019 •

edited

rjgotten commented Jul 23, 2019

alexander-akait commented Jul 23, 2019 •

edited

rjgotten commented Jul 24, 2019 •

edited

alexander-akait left a comment

alexander-akait Jul 24, 2019

rjgotten Jul 24, 2019

alexander-akait Jul 24, 2019

rjgotten Jul 24, 2019

alexander-akait Jul 24, 2019

rjgotten Jul 24, 2019 •

edited

rjgotten Jul 24, 2019

alexander-akait Jul 24, 2019

rjgotten Jul 24, 2019

alexander-akait Jul 24, 2019

rjgotten Jul 24, 2019

rjgotten Jul 24, 2019

rjgotten commented Jul 30, 2019

alexander-akait commented Jul 30, 2019

ron23 commented May 13, 2020

jimblue commented May 28, 2020

rjgotten commented May 29, 2020

jimblue commented May 29, 2020

jimblue commented Jun 2, 2020

feat: hmr error recovery #435

Are you sure you want to change the base?

feat: hmr error recovery #435

Conversation

rjgotten commented Jul 23, 2019 • edited

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented Jul 23, 2019 • edited

rjgotten commented Jul 23, 2019 • edited

Choose a reason for hiding this comment

rjgotten Jul 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

rjgotten commented Jul 23, 2019 • edited

rjgotten commented Jul 23, 2019 • edited

alexander-akait commented Jul 23, 2019 • edited

rjgotten commented Jul 23, 2019

alexander-akait commented Jul 23, 2019 • edited

rjgotten commented Jul 24, 2019 • edited

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjgotten Jul 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjgotten commented Jul 30, 2019

alexander-akait commented Jul 30, 2019

ron23 commented May 13, 2020

jimblue commented May 28, 2020

rjgotten commented May 29, 2020

jimblue commented May 29, 2020

jimblue commented Jun 2, 2020

rjgotten commented Jul 23, 2019 •

edited

jsf-clabot commented Jul 23, 2019 •

edited

rjgotten commented Jul 23, 2019 •

edited

rjgotten Jul 23, 2019 •

edited

rjgotten commented Jul 23, 2019 •

edited

rjgotten commented Jul 23, 2019 •

edited

alexander-akait commented Jul 23, 2019 •

edited

alexander-akait commented Jul 23, 2019 •

edited

rjgotten commented Jul 24, 2019 •

edited

rjgotten Jul 24, 2019 •

edited