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

Bug: ErrorBoundary won't caught error in useEffect callback while ErrorBoundary unmount with it's children #25192

Closed
Chen-jj opened this issue Sep 6, 2022 · 8 comments
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@Chen-jj
Copy link

Chen-jj commented Sep 6, 2022

React version: 18.2.0

Steps To Reproduce

function App() {
  const [isShow, setIsShow] = React.useState(true);
  function setError() {
    setIsShow(false);
  }
  return (
    <div>
      {isShow && <PageWrapper />}
      <div onClick={setError}>setError</div>
    </div>
  );
}

class PageWrapper extends React.Component {
  static getDerivedStateFromError() {}
  componentDidCatch(err) {
    console.log("catch err: ", err);
  }
  render() {
    return <Page />;
  }
}

function Page() {
  React.useEffect(() => {
    return () => {
      throw new Error("sorry!");
    };
  });
  return <div>I m page</div>;
}

const root = ReactDOM.createRoot(document.getElementById('root'));
root.render(<App />)
  1. click setError.Unmount <PageWrapper> along with <Page>, then <Page> threw an error in useEffect callback.

The current behavior

ErrorBoundary <PageWrapper> won't caught error thrown by <Page> useEffect callback.So all the React Tree is dropped.

But in React 17, <PageWrapper> caught the error.And the React Tree remain exists.

The expected behavior

Catch the error like React 17.

So, is that React18 new behavior? Or is a bug?

@sadiki-o
Copy link

sadiki-o commented Sep 6, 2022

can you please tell me why are you mixing class components with function components?

@Chen-jj
Copy link
Author

Chen-jj commented Sep 6, 2022

sadiki-o

Because ErrorBoundary only work with class components.

@sadiki-o
Copy link

sadiki-o commented Sep 6, 2022

oh my apology since i worked mostly with function components i never get to use such feature

@creamidea
Copy link

creamidea commented Sep 13, 2022

This may be related to the change in the commitRoot phase traversal. v17 traverses the effect list. v18 traverses the entire tree.
When dealing with PassiveUnmountEffects, the connection to child is removed. v18 does not have a way to call PageWrapper's Page's destroy, while v17 can handle the destroy function through the effect list.

@creamidea
Copy link

creamidea commented Sep 13, 2022

Maybe I find the problem, here is set skipUnmountedBoundaries to be true.

if (skipUnmountedBoundaries) {

So, the compiler's output

image

image

Why?

// This rolled out to 10% public in www, so we should be able to land, but some

// This rolled out to 10% public in www, so we should be able to land, but some
// internal tests need to be updated. The open source behavior is correct.
export const skipUnmountedBoundaries = true;

@tgohn
Copy link

tgohn commented Feb 24, 2023

I did encountered the same error as mentioned.
Digging through PRs history, the above behaviour change in ErrorBoundary is expected. The details were described in PR #19627

There were quite a number of commits and reverts related to this change.
I could be wrong, but I think these are the main ones:

  • The original PR that started this ErrorBoundary change. There are helpful discussions here.
  • The PR that moves the above change under skipUnmountedBoundaries feature flag: default to off
  • The PRs (1, 2) that enable the flag to true always. This lands in React v18 public.

I wish the change in ErrorBoundary has been mentioned in React v18 changelog.

Hope this helps.

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

4 participants