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

Runtime error overlay does not disappear when fixed with HMR #3096

Closed
eirikurn opened this issue Sep 8, 2017 · 12 comments
Closed

Runtime error overlay does not disappear when fixed with HMR #3096

eirikurn opened this issue Sep 8, 2017 · 12 comments
Milestone

Comments

@eirikurn
Copy link

eirikurn commented Sep 8, 2017

Is this a bug report?

Yes

Can you also reproduce the problem with npm 4.x?

Yes

Which terms did you search for in User Guide?

Hot module replacement. Error overlay.

Environment

  1. node -v: 8.1.4
  2. npm -v: 5.3.0
  3. yarn --version: 1.0.1
  4. npm ls react-scripts: 1.0.13

Then, specify:

  1. Operating system: macOS 10.12.6
  2. Browser and version (if relevant): Chrome 60

Steps to Reproduce

  1. Add hot module reloading by accepting hot changes in src/index.js which re-renders the React app.
  2. Add a runtime exception in some React components. (throw new Error()).
  3. Open app to see the error overlay.
  4. Fix the error in your React component.
  5. Wait for webpack to rebuild and look at the app.

Expected Behavior

The error overlay should disappear.

Actual Behavior

The error overlay stays in view. If I close the overlay, I can see the new state of the app, properly fixed.

@eirikurn
Copy link
Author

eirikurn commented Sep 8, 2017

After writing this, I realize that there's no way to know if the runtime error has been fixed and properly replaced with HMR. The HMR could have been a simple css change, unrelated to the runtime error.

Feel free to close if you don't think this is an issue.

@Timer
Copy link
Contributor

Timer commented Sep 8, 2017

What we need to figure out and ensure happens is that a HMR is declined after a runtime error so a full reload is required (and automatic).

Does this sound reasonable to you?

@eirikurn
Copy link
Author

eirikurn commented Sep 8, 2017

Yes, I think that would be a great 80/20 solution.

@gaearon
Copy link
Contributor

gaearon commented Sep 8, 2017

Did this work before? I remember that we wanted this to work (force refreshing after any errors) but I'm not sure if it did.

@Timer
Copy link
Contributor

Timer commented Sep 8, 2017

Looks like the code is still there, so I assume it never functioned the way we intended:
https://github.com/facebookincubator/create-react-app/blob/fcb6dc55578e089e9f62f6e6c12cb9b60a57fee5/packages/react-dev-utils/webpackHotDevClient.js#L28-L37

@gaearon
Copy link
Contributor

gaearon commented Sep 8, 2017

I wonder if it worked before the refactoring.

@eirikurn
Copy link
Author

eirikurn commented Sep 8, 2017

I saw this issue in the most recent react-dev-utils 3.x and now after upgrading to react-dev-utils 4.

@gaearon
Copy link
Contributor

gaearon commented Sep 8, 2017

So it probably worked. We probably broke it in #2515. (cc @tharakawj)

@gaearon gaearon modified the milestones: 1.0.13, 2.0.0 Sep 8, 2017
@Timer
Copy link
Contributor

Timer commented Sep 8, 2017

To be sure, can you check if this worked in 1.0.11 @eirikurn?

@eirikurn
Copy link
Author

eirikurn commented Sep 8, 2017

Hm. In both versions, the page does two full refreshes if there's an error during the render (which runs as part of module.hot.accept(...)):

  1. I have the page open.
  2. Add to the render function of some component window.foo.bar.
  3. HMR runs, render fails, page fully reloads, render fails again and finally error-overlay is shown.
  4. Fix the issue from step 2.
  5. HMR does a full reload and page renders correctly (with no error-overlay).

However, if the runtime error is somewhere else, e.g. in an event handler, the behaviour in the OP appears. No page reloads, overlay stays up.

@Timer
Copy link
Contributor

Timer commented Sep 8, 2017

Ok, so this wasn't a regression. Thanks for the information, @eirikurn!

@Timer
Copy link
Contributor

Timer commented Sep 8, 2017

Fix up in #3098; basically forces a reload after the error overlay has been shown.

Timer added a commit that referenced this issue Sep 9, 2017
* Reload the page when an error has occurred
Fixes #3096

* Use a global boolean instead
thongdong7 pushed a commit to thongdong7/create-react-app that referenced this issue Sep 24, 2017
* Reload the page when an error has occurred
Fixes facebook#3096

* Use a global boolean instead
thongdong7 pushed a commit to thongdong7/create-react-app that referenced this issue Sep 24, 2017
* Reload the page when an error has occurred
Fixes facebook#3096

* Use a global boolean instead
kasperpeulen pushed a commit to kasperpeulen/create-react-app that referenced this issue Sep 24, 2017
* Reload the page when an error has occurred
Fixes facebook#3096

* Use a global boolean instead
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants