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

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

Merged
merged 6 commits into from Aug 13, 2018

Conversation

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 13, 2018

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:

  1. If we're in development mode, and
  2. If an error was successfully caught by an error boundary, and
  3. If the user has called event.preventDefault() for that error in a custom error event handler, then
  4. 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).

gaearon added 3 commits Aug 13, 2018
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
Copy link

@pull-bot pull-bot commented Aug 13, 2018

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

Loading

Copy link
Member

@acdlite acdlite left a comment

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

Loading

typeof WeakSet === 'function' ? new WeakSet() : null;

(ReactErrorUtils: any).markErrorAsSuppressedInDEV = function(error: any) {
const suppressedErrors = (ReactErrorUtils: any)._suppressedErrorsInDEV;
Copy link
Member

@acdlite acdlite Aug 13, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member

@acdlite acdlite Aug 13, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@gaearon gaearon Aug 13, 2018

Choose a reason for hiding this comment

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

You could throw a frozen object though.

Loading

Copy link
Member Author

@gaearon gaearon Aug 13, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member

@acdlite acdlite Aug 13, 2018

Choose a reason for hiding this comment

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

Yeah seems fine

Loading

@gaearon gaearon force-pushed the errors-less-noise branch from 7899db0 to c433932 Aug 13, 2018
Copy link
Member

@acdlite acdlite left a comment

Cool!

Loading

@gaearon gaearon merged commit 3938ccc into facebook:master Aug 13, 2018
1 check was pending
Loading
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
…cebook#13384)

* Remove e.suppressReactErrorLogging check before last resort throw

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

* Add fixtures

* Use an expando property instead of a WeakSet

* Make it a bit less fragile

* Clarify comments
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants