Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: hmr error recovery#435

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
rjgotten wants to merge5 commits intowebpack-contrib:master
base:master
Choose a base branch
Loading
fromNetMatch:hmr-error-recovery

Conversation

rjgotten
Copy link

@rjgottenrjgotten commentedJul 23, 2019•
edited
Loading

This PR contains a:

  • bugfix
  • newfeature
  • code refactor
  • 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 tohot.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.

Itfixes#431

Breaking Changes

None, as far as I am aware. All existing test scenarios in the suite continue to pass.

Additional Info

TheNetMatch/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
Copy link

jsf-clabot commentedJul 23, 2019•
edited
Loading

CLA assistant check
All committers have signed the CLA.

@rjgotten
Copy link
Author

rjgotten commentedJul 23, 2019•
edited
Loading

Not quite sure what's up here; Idid 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.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

For catching errors, right?

Copy link
Author

@rjgottenrjgottenJul 23, 2019•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

@alexander-akaitalexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@rjgotten
Copy link
Author

rjgotten commentedJul 23, 2019•
edited
Loading

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

alexander-akait and jimblue reacted with thumbs up emoji

@rjgotten
Copy link
Author

rjgotten commentedJul 23, 2019•
edited
Loading

@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 usingexpress withwebpack-dev-middleware andwebpack-hot-middleware
  • A browser that can be instrumented, e.g. usingpuppeteer
  • 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:

  1. Start the Express server (and the webpack compilation)
  2. Boot up Puppeteer and load the testcaseindex.html page
  3. Start logging console output from Puppeteer.
  4. Wait for the "[HMR] Connected" message
  5. Change a source file on disk leading to a compilation error (e.g. introduce a syntax error into a Less or Sass file)
  6. Wait for the batch of "[HMR]" messages to come in from Puppeteer
  7. Match against expected output.
  8. etc. etc. following through the entire test scenario.
  9. 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 theinput 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'smemory-fs does not have. Seems definitely out-of-scope.

@alexander-akait
Copy link
Member

alexander-akait commentedJul 23, 2019•
edited
Loading

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

@rjgotten
Copy link
Author

You don't need this stuff, just mockless-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
Copy link
Member

alexander-akait commentedJul 23, 2019•
edited
Loading

@rjgotten this enough

@rjgotten
Copy link
Author

rjgotten commentedJul 24, 2019•
edited
Loading

@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 emitshot bindings before throwing its error.

Of special note:
I had to introduce a convention on theTestCases.test.js that processes the/cases/ folder. That convention is that cases starting with a~ areexpected 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, whichvaries by system
Copy link
Member

@alexander-akaitalexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please look on notes

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Don't use~ in filename

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@rjgotten just usefail word in test name

rjgotten reacted with thumbs up emoji
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why it is necessary for test?

Copy link
Author

@rjgottenrjgottenJul 24, 2019•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A little more context please:
whywhat ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done

- 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
Copy link
Author

@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
Copy link
Member

in todo list

rjgotten and jimblue reacted with thumbs up emoji

@ron23
Copy link

Hi, any progress on this?

jimblue reacted with thumbs up emoji

@jimblue
Copy link

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

@rjgotten
Copy link
Author

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 insrc/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 reacted with thumbs up emoji

@jimblue
Copy link

@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
Copy link

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

liamsc reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@alexander-akaitalexander-akaitalexander-akait requested changes

@ScriptedAlchemyScriptedAlchemyScriptedAlchemy left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HMR stops updating after first error, reports as unaccepted module
6 participants
@rjgotten@jsf-clabot@alexander-akait@ron23@jimblue@ScriptedAlchemy
[8]ページ先頭
©2009-2025 Movatter.jp