Skip to content

Suggestion: Don't display error iframe #28

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

Closed
fwh1990 opened this issue Dec 20, 2019 · 23 comments · Fixed by #44
Closed

Suggestion: Don't display error iframe #28

fwh1990 opened this issue Dec 20, 2019 · 23 comments · Fixed by #44

Comments

@fwh1990
Copy link

fwh1990 commented Dec 20, 2019

Sometimes, the Promise reject data which is expected, but this plugin report error message by create an iframe#react-refresh-overlay element to html.body. That's boring, because I can see error message in Console.

This behavior will interrupted my mind and I have to close iframe by hand.

@pmmmwh
Copy link
Owner

pmmmwh commented Dec 20, 2019

Sometimes, the Promise reject data which is expected, but this plugin report error message by create an iframe#react-refresh-overlay element to html.body. That's boring, because I can see error message in Console.

This behavior will interrupted my mind and I have to close iframe by hand.

I thought the overlay only shows up when there are unhandled promise rejections? That's an Error by definition, isn't it? Or are you saying that it shows up even when the rejection is handled?

@fwh1990
Copy link
Author

fwh1990 commented Dec 20, 2019

I didn't handle promise rejection, that's my purpose. The program work well without handle rejection. Would you please add an option to disable this behavior?

@fwh1990
Copy link
Author

fwh1990 commented Dec 20, 2019

I just tested Promise.reject in RN 0.61, it's reported as a warning display at the bottom of screen. You recognize it as error.

Whatever, I also want to have an option to disable showing error.

@pmmmwh
Copy link
Owner

pmmmwh commented Dec 22, 2019

My thoughts on this:

  • Unhandled promise rejection should be warned as it is being warned in the console anyways. I can tune it down to be an error, but I wouldn't want to allow someone to switch of unhandled rejections independently in the error overlay
  • Realistically, I can allow an option to make the plugin not inject the error overlay runtime code, which will effectively disable the overlay. I can see this useful in a few ways, for example custom HMR implementations or custom error logic. (also would be useful for CRA since they have both of these)

CC @gaearon your thoughts?

@fwh1990
Copy link
Author

fwh1990 commented Dec 28, 2019

Any progress?

@pmmmwh
Copy link
Owner

pmmmwh commented Dec 28, 2019

Any progress?

I'm currently on vacation and don't have much free time to do anything yet. You can submit a PR if you want this done quicker.

@mattfwood
Copy link

For cases where you need to ignore a thrown exception, you should have something in your user code that is catching it and doing something with it (you could also use an Error Boundary):

try {
  const res = await api.post('data');
  return res.data;
} catch (error) {
  // if the promise is rejected
  console.log(error); // log out the error, but catching here prevents this from being an uncaught exception
  // do something to display error message
}

Keep in mind that in React, if you hit an uncaught exception it will crash the entire app. From the React Docs:

This change has an important implication. As of React 16, errors that were not caught by any error boundary will result in unmounting of the whole React component tree.

This feature you are asking for is not just about development experience. It means that even if you ignored the error in development mode, it will still crash the React component tree.

@fwh1990
Copy link
Author

fwh1990 commented Dec 31, 2019

Keep in mind that in React, if you hit an uncaught exception it will crash the entire app.

My project run well when I am using react-hot-loader, and never ever crash when I uncaught rejection. I don't think Promise.reject can crash React component tree, it's async and out of control for react

@mattfwood
Copy link

Ahh, my mistake, i was confusing rejected promises with uncaught exceptions.

@gaearon
Copy link

gaearon commented Jan 7, 2020

Error overlay should display unhandled rejections. It is easy to intentionally handle it by adding catch() if you don't want to treat it as an error.

It is also true that we should offer a way to customize the error overlay to "redirect" it to another module. We'll need need this for opinionated integrations like Next. Seems like that would cover the use case for a "noop" overlay too anyway.

@SimenB
Copy link

SimenB commented Jan 7, 2020

Using the builtin error overlay in webpack creates duplicate overlays with this plugin active. I don't think it's a single plugins responsibility to provide an overlay, it should be explicitly configured or provided as part of an abstraction like CRA, Next etc.

@gaearon
Copy link

gaearon commented Jan 14, 2020

Using the builtin error overlay in webpack creates duplicate overlays with this plugin active.

Are you talking about the compile-time or runtime overlay?

@SimenB
Copy link

SimenB commented Jan 14, 2020

Compile-time.

EDIT: Wait, I get double error overlay for both

@OneCyrus
Copy link

yes, please let us disable it. the visualization is not that great. we already use error-overlay-webpack-plugin which give better errors.

image

this is far better for us:

image

@githoniel
Copy link

webpack-dev-server provide builtin overlay already.
https://webpack.js.org/configuration/dev-server/#devserveroverlay
I think it is enough for most case, and this plugin should have an options to disable the layout to avoid duplicate

@mmhand123
Copy link
Contributor

I made a PR to make the overlay default off and added an option to add it back in. Wonder though if it might be better for the plugin to not worry about the overlay at all.

@SimenB
Copy link

SimenB commented Jan 16, 2020

I think it shouldn't be here at all, but as long as it's not forced upon me it doesn't really bother me

@IronSean
Copy link

We just hit a situation where the overlay makes our app unusable and prevents testing and development. I think maybe it shouldn't sorry about it at all, but even if you're unsure about that please merge the pr defaulting it to off while you decide.

@IronSean
Copy link

@pmmmwh ^^

@pmmmwh
Copy link
Owner

pmmmwh commented Feb 21, 2020

We just hit a situation where the overlay makes our app unusable and prevents testing and development. I think maybe it shouldn't sorry about it at all, but even if you're unsure about that please merge the pr defaulting it to off while you decide.

Hi, sorry that I have been very busy lately. I'll take a look at it tomorrow and provide my thoughts on that.

@cameronscottseequent
Copy link

For anyone that stumbles upon this.

supply this setting in your webpack config file

plugins: [new ReactRefreshWebpackPlugin({overlay: false})],

@globad
Copy link

globad commented Aug 24, 2022

cameronscottseequent Thank you! That helped me.

@okovalov
Copy link

@cameronscottseequent thank you so much! that worked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet