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

Show error overlay even after browser refresh #132

Merged
merged 7 commits into from
Oct 6, 2016

Conversation

richardscarrott
Copy link
Contributor

@richardscarrott richardscarrott commented Oct 5, 2016

Currently, if you refresh the client whilst the compilation is in an error state you don't get any feedback (and if using the NoErrorsPlugin you actually get a working app; albeit using the last working bundle) -- you need to check the terminal window to be sure this hasn't happened.

This PR aims to solve the problem by publishing the 'built' event based on the last known compilation stats as soon as a client is registered with the middleware meaning the client script will show the overlay if appropriate.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling 0c9aabf on 60frames:refresh into ca9871d on glenjamin:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling 0c9aabf on 60frames:refresh into ca9871d on glenjamin:master.

Copy link
Collaborator

@glenjamin glenjamin left a comment

Choose a reason for hiding this comment

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

This seems sensible to me. Are there any potential problems do you think with having the other bits of the hot reloading code run?

@richardscarrott
Copy link
Contributor Author

richardscarrott commented Oct 5, 2016

@glenjamin I think it's actually desirable to have the rest of the hot reloading code run because as soon as you fix the error it'll just continue as if you hadn't ever refreshed.

I'll tidy it up and try to ensure I cover any potential edge cases.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage decreased (-0.2%) to 75.839% when pulling 22d1f1b on 60frames:refresh into ca9871d on glenjamin:master.

@richardscarrott richardscarrott changed the title [WIP] Show error overlay even after browser refresh Show error overlay even after browser refresh Oct 5, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling 443ffa9 on 60frames:refresh into ca9871d on glenjamin:master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling 443ffa9 on 60frames:refresh into ca9871d on glenjamin:master.

@richardscarrott
Copy link
Contributor Author

@glenjamin I've tidied it up with the following improvements:

  • Added a 'sync' action to enable the client to differentiate it from the 'built' action; this allows the browser console logs to be nicer, i.e. it won't log [HMR] bundle rebuilt in 6649ms every time you refresh the client of an already built bundle.
  • No longer logs webpack built e484f8b43f0bc5beada4 in 32151ms whenever a request is received on the server -- i.e. the server already knows about this from the 'built' event so it doesn't log on the 'sync' event.
  • Added tests

Let me know your thoughts.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling b94bf73 on 60frames:refresh into ca9871d on glenjamin:master.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling cd2abab on 60frames:refresh into ca9871d on glenjamin:master.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling cdc3c34 on 60frames:refresh into ca9871d on glenjamin:master.

sinon.assert.calledOnce(processUpdate);
it("should trigger webpack on successful builds / syncs", function() {
const actions = ['built', 'sync'];
actions.forEach(function(action) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you move this loop outside the it(), or manually repeat the test so that it's two distinct cases?

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.5%) to 76.51% when pulling 0ea4528 on 60frames:refresh into ca9871d on glenjamin:master.

@glenjamin glenjamin merged commit 1da10eb into webpack-contrib:master Oct 6, 2016
@richardscarrott
Copy link
Contributor Author

@glenjamin 👍 let me know when you next publish

@glenjamin
Copy link
Collaborator

Released as 2.13.0

joshwiens pushed a commit that referenced this pull request Mar 30, 2018
* Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh
joshwiens pushed a commit that referenced this pull request Mar 30, 2018
* Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh

* fixup! Show error overlay even after browser refresh
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 this pull request may close these issues.

3 participants