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

ErrorBoundary rendering multiple copies of itself when ref assignment fails #16365

Open
silverwind opened this issue Aug 12, 2019 · 6 comments
Open

Comments

@silverwind
Copy link

silverwind commented Aug 12, 2019

Do you want to request a feature or report a bug?

bug

What is the current behavior?

When a error occurs during the assignment of a ref (and maybe other conditions), a error boundary wrapping that error may get confused and it renders itself multiple times inside the same parent. See https://codesandbox.io/s/stoic-fermi-6etqb which renders:

<div id="root">
  <div class="boundary"><span>content</span></div>
  <div class="boundary"><span>error</span></div>
</div>

What is the expected behavior?

<div id="root">
  <div class="boundary"><span>error</span></div>
</div>
@silverwind silverwind changed the title ErrorBoundary rendering multiple copies of itself ErrorBoundary rendering multiple copies of itself when ref assignment fails Aug 12, 2019
@gaearon
Copy link
Collaborator

gaearon commented Aug 15, 2019

Is this a new bug?

@silverwind
Copy link
Author

silverwind commented Aug 15, 2019

Flipping through the versions on that sandbox, it first appears in react-dom@16.3.0-alpha.1.

@fireflyEngineer
Copy link

Is this issue still opened ? If so I'll have a try.

@dhrubesh
Copy link

dhrubesh commented Oct 7, 2019

@gaearon Mind if I take this up?

@GasimGasimzada
Copy link
Contributor

GasimGasimzada commented Jan 2, 2020

Shouldn't you be using getDerivedStateFromError instead of componentDidCatch to set the error state:

class ErrorBoundary extends React.Component {
  state = { err: null };

  static getDerivedStateFromProps(err) {
    return { err };
  }
  
  // componentDidCatch = err => this.setState({ err });
  // ^ This should only be used for side effects. E.g logging errors etc

  render() {
    return (
      <div className="boundary">
        {this.state.err ? <span>error</span> : this.props.children}
      </div>
    );
  }
}

EDIT: https://codesandbox.io/s/brave-mahavira-g9n5x

@silverwind
Copy link
Author

Shouldn't you be using getDerivedStateFromError instead of componentDidCatch

Ideally both should work but I just tested getDerivedStateFromError and it does not seem to be affected by this issue.

See https://codesandbox.io/s/recursing-dawn-l029b for the getDerivedStateFromError version. Flip lines 7 and 8 in it for both versions. The Error overlay needs to be manually dismissed in the componentDidCatch version to see the double rendering.

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

Successfully merging a pull request may close this issue.

5 participants