Allow the user to opt out of seeing "The above error..." addendum #13384

gaearon · 2018-08-13T19:15:08Z

This partially addresses #11098.

Currently, when we have an error in development, we later print an error addendum:

The above error occurred in <BadRenderComponent>.
Consider adding an error boundary...

This creates a lot of noise in tests for error boundaries, i.e. when you expect that an error would be thrown and handled. The original error also gets reported by jsdom even if it's wrapped in a catch block. Just like in the browser. So you end up with two console.error warnings you have to mock or ignore.

However, both browsers and jsdom already provide a way to suppress the error message:

window.addEventListener('error', e => {
  e.preventDefault();
});

In this case neither the browser nor jsdom print the original error. But we still print our addendum which is now taken out of context (it says "The above error occurred" — but there's no above error!)

In this PR, I'm making React respect e.preventDefault() in the window handler of the error event in a browser environment in some circumstances.

In particular:

If we're in development mode, and
If an error was successfully caught by an error boundary, and
If the user has called event.preventDefault() for that error in a custom error event handler, then
We won't show the "The above error occurred..." addendum.

We still show it if the error was fatal (either because you didn't have a boundary or because the boundary failed). This is to reduce the risk of accidental silencing of errors with a rogue error handler.

What about the cases like #11098 (comment) where you don't have a boundary but still want to avoid warning noise? The solution I'd recommend is to create a boundary, and test that it caught the error.

So how would you use this in tests?

Consider this App component.

import React, { Component } from 'react';

class Crash extends Component {
  componentDidMount() {
    throw new Error('noo');
  }
  render() {
    return <h1>hi</h1>
  }
}

class Boundary extends Component {
  state = {failed: false};

  componentDidCatch() {
    this.setState({
      failed: true
    })
  }

  render() {
    if (this.state.failed) {
      return 'oopsie';
    }
    return (
      <div>
        {this.props.children}
      </div>
    );
  }
}

class App extends Component {
  render() {
    return (
      <Boundary>
        <Crash />
      </Boundary>
    );
  }
}

export default App;

Previously you couldn't test that error boundary worked without mocking console.error. Now you can:

import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';

let caughtErrors = [];
function onError(e) {
  caughtErrors.push(e.error);
  e.preventDefault();
}

beforeEach(() => {
  caughtErrors.length = 0;
  window.addEventListener('error', onError);
});

afterEach(() => {
  window.removeEventListener('error', onError);
  caughtErrors.length = 0;
});

it('renders without crashing', () => {
  const div = document.createElement('div');
  ReactDOM.render(<App />, div);
  ReactDOM.unmountComponentAtNode(div);
  expect(caughtErrors.length).toBe(1);
  expect(caughtErrors[0].message).toContain('noo');
});

This already wouldn't print the browser error, but with this change, it doesn't print our addendum either.

There is one final detail. If the error was fatal, I think we should still show it. The problem is that if the user silenced it with preventDefault(), our addendum is a bit useless because it doesn't contain the error itself — only the stack. So I changed this codepath to also emit the error object if it has been silenced via a separate console.error() call prior to the addendum log. This mimics what the browser does normally, and I think is a reasonable precaution against badly behaved error handlers. If you want to avoid this, add error boundaries and you'll be good.

I added some DOM fixtures to verify the new behavior.

These changes have no effect on production or non-DOM code paths. This means that people using test renderer or running ReactDOM in production mode would still have to mock console.error in tests. But I think it's reasonable to solve the most common case — and for console.error, it's less of a hassle to mock when there's only one of them, and it matches the error exactly (as opposed to jsdom / React emitting two different error logs).

It's unnecessary here. It was here because this method called console.error(). But we now rethrow with a clean stack, and that's worth doing regardless of whether the logging is silenced.

pull-bot · 2018-08-13T19:25:00Z

ReactDOM: size: -0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: 47e217a...2b22524

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.2%	+0.4%	641.8 KB	643.37 KB	150.86 KB	151.41 KB	UMD_DEV
react-dom.production.min.js	-0.0%	0.0%	96.31 KB	96.28 KB	31.21 KB	31.21 KB	UMD_PROD
react-dom.development.js	+0.2%	+0.4%	637.94 KB	639.5 KB	149.71 KB	150.26 KB	NODE_DEV
react-dom.production.min.js	-0.0%	-0.0%	96.3 KB	96.27 KB	30.79 KB	30.79 KB	NODE_PROD
ReactDOM-dev.js	+0.2%	+0.3%	645.1 KB	646.22 KB	147.94 KB	148.36 KB	FB_WWW_DEV
ReactDOM-prod.js	-0.0%	-0.0%	279.06 KB	279.01 KB	52.29 KB	52.28 KB	FB_WWW_PROD
react-dom.profiling.min.js	-0.0%	-0.0%	97.51 KB	97.48 KB	31.19 KB	31.19 KB	NODE_PROFILING
ReactDOM-profiling.js	-0.0%	-0.0%	281.47 KB	281.42 KB	52.93 KB	52.92 KB	FB_WWW_PROFILING

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.4%	+0.6%	433.29 KB	434.85 KB	98.16 KB	98.71 KB	UMD_DEV
react-art.production.min.js	-0.0%	-0.0%	85.19 KB	85.16 KB	26.41 KB	26.4 KB	UMD_PROD
react-art.development.js	+0.4%	+0.7%	365.81 KB	367.37 KB	81.11 KB	81.65 KB	NODE_DEV
react-art.production.min.js	-0.1%	-0.0%	50.18 KB	50.15 KB	15.8 KB	15.79 KB	NODE_PROD
ReactART-dev.js	+0.3%	+0.5%	356.17 KB	357.29 KB	76 KB	76.39 KB	FB_WWW_DEV
ReactART-prod.js	-0.0%	-0.0%	153.24 KB	153.2 KB	26.43 KB	26.42 KB	FB_WWW_PROD

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.4%	+0.7%	361.7 KB	363.27 KB	79.5 KB	80.05 KB	UMD_DEV
react-test-renderer.production.min.js	-0.1%	-0.0%	49.06 KB	49.03 KB	15.17 KB	15.16 KB	UMD_PROD
react-test-renderer.development.js	+0.4%	+0.7%	357.83 KB	359.4 KB	78.54 KB	79.08 KB	NODE_DEV
react-test-renderer.production.min.js	-0.1%	-0.0%	48.77 KB	48.74 KB	15.02 KB	15.02 KB	NODE_PROD
ReactTestRenderer-dev.js	+0.3%	+0.5%	362.83 KB	363.95 KB	77.5 KB	77.9 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.5%	+0.7%	346.82 KB	348.39 KB	75.15 KB	75.71 KB	NODE_DEV
react-reconciler.production.min.js	-0.1%	-0.0%	47.75 KB	47.71 KB	14.47 KB	14.46 KB	NODE_PROD
react-reconciler-persistent.development.js	+0.5%	+0.7%	345.44 KB	347 KB	74.6 KB	75.16 KB	NODE_DEV
react-reconciler-persistent.production.min.js	-0.1%	-0.0%	47.76 KB	47.72 KB	14.47 KB	14.47 KB	NODE_PROD

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.3%	+0.5%	480.99 KB	482.56 KB	106.33 KB	106.86 KB	RN_FB_DEV
ReactNativeRenderer-prod.js	-0.0%	-0.1%	213.39 KB	213.34 KB	37.51 KB	37.49 KB	RN_FB_PROD
ReactNativeRenderer-dev.js	+0.3%	+0.5%	480.73 KB	482.29 KB	106.27 KB	106.8 KB	RN_OSS_DEV
ReactNativeRenderer-prod.js	-0.0%	-0.1%	203.44 KB	203.39 KB	35.89 KB	35.87 KB	RN_OSS_PROD
ReactFabric-dev.js	+0.3%	+0.5%	471.21 KB	472.77 KB	103.88 KB	104.42 KB	RN_FB_DEV
ReactFabric-prod.js	-0.0%	-0.1%	195.71 KB	195.67 KB	34.38 KB	34.36 KB	RN_FB_PROD
ReactFabric-dev.js	+0.3%	+0.5%	471.24 KB	472.8 KB	103.9 KB	104.43 KB	RN_OSS_DEV
ReactFabric-prod.js	-0.0%	-0.1%	195.75 KB	195.7 KB	34.4 KB	34.38 KB	RN_OSS_PROD
ReactNativeRenderer-profiling.js	-0.0%	-0.1%	206.86 KB	206.82 KB	36.54 KB	36.52 KB	RN_OSS_PROFILING
ReactFabric-profiling.js	-0.0%	-0.1%	198.72 KB	198.67 KB	34.95 KB	34.93 KB	RN_OSS_PROFILING
ReactNativeRenderer-profiling.js	-0.0%	-0.1%	216.78 KB	216.73 KB	38.15 KB	38.12 KB	RN_FB_PROFILING
ReactFabric-profiling.js	-0.0%	-0.1%	198.68 KB	198.63 KB	34.93 KB	34.91 KB	RN_FB_PROFILING

Generated by 🚫 dangerJS

acdlite

Did you consider using an expando instead of a Set/WeakSet?

acdlite · 2018-08-13T19:34:38Z

packages/shared/ReactErrorUtils.js

+    typeof WeakSet === 'function' ? new WeakSet() : null;
+
+  (ReactErrorUtils: any).markErrorAsSuppressedInDEV = function(error: any) {
+    const suppressedErrors = (ReactErrorUtils: any)._suppressedErrorsInDEV;


Will an expando no longer work? error._suppressLogging = true

If we use an expando, then I think you can get rid of the extra ReactErrorUtils methods.

You could throw a frozen object though.

I guess it's uncommon enough that I can try/catch around it?

Yeah seems fine

acdlite

Cool!

gaearon added 3 commits August 13, 2018 16:18

Remove e.suppressReactErrorLogging check before last resort throw

b1009e8

It's unnecessary here. It was here because this method called console.error(). But we now rethrow with a clean stack, and that's worth doing regardless of whether the logging is silenced.

Don't print error addendum if 'error' event got preventDefault()

dd9f09d

Add fixtures

977ae18

gaearon requested a review from acdlite August 13, 2018 19:15

facebook-github-bot added the CLA Signed label Aug 13, 2018

acdlite reviewed Aug 13, 2018

View reviewed changes

Use an expando property instead of a WeakSet

c433932

gaearon force-pushed the errors-less-noise branch from 7899db0 to c433932 Compare August 13, 2018 20:17

acdlite approved these changes Aug 13, 2018

View reviewed changes

gaearon added 2 commits August 13, 2018 21:21

Make it a bit less fragile

da9d6f6

Clarify comments

2b22524

gaearon merged commit 3938ccc into facebook:master Aug 13, 2018

gaearon mentioned this pull request Aug 13, 2018

Allow suppressing error boundary logs from intentionally thrown/caught errors #11098

Closed

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

dependencies bot mentioned this pull request Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

dependencies bot mentioned this pull request Nov 1, 2018

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the user to opt out of seeing "The above error..." addendum #13384

Allow the user to opt out of seeing "The above error..." addendum #13384

gaearon commented Aug 13, 2018 •

edited

pull-bot commented Aug 13, 2018 •

edited

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

acdlite left a comment

acdlite Aug 13, 2018 •

edited

acdlite Aug 13, 2018

gaearon Aug 13, 2018

gaearon Aug 13, 2018

acdlite Aug 13, 2018

acdlite left a comment

Allow the user to opt out of seeing "The above error..." addendum #13384

Allow the user to opt out of seeing "The above error..." addendum #13384

Conversation

gaearon commented Aug 13, 2018 • edited

pull-bot commented Aug 13, 2018 • edited

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

acdlite left a comment

Choose a reason for hiding this comment

acdlite Aug 13, 2018 • edited

Choose a reason for hiding this comment

acdlite Aug 13, 2018

Choose a reason for hiding this comment

gaearon Aug 13, 2018

Choose a reason for hiding this comment

gaearon Aug 13, 2018

Choose a reason for hiding this comment

acdlite Aug 13, 2018

Choose a reason for hiding this comment

acdlite left a comment

Choose a reason for hiding this comment

gaearon commented Aug 13, 2018 •

edited

pull-bot commented Aug 13, 2018 •

edited

acdlite Aug 13, 2018 •

edited