facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.88k stars 47.06k forks source link

Bug: Nested lazy components cause rerendering #29235

Closed raymondwang closed 17 hours ago

raymondwang commented 6 months ago

This is a duplicate of https://github.com/facebook/react/issues/27573, which was closed without response. We're noticing this issue at scale, and it's fairly pronounced, with hundreds of rapid re-renders triggered by a single lazy component. I've forked the replication from original issue to demonstrate that this is still an issue in React 19:

https://codesandbox.io/p/sandbox/react-lazy-rerender-bug-forked-q4q6ym

The current behavior

The lazy parent component renders multiple times:

Screenshot 2024-05-23 at 10 54 06 AM

(In our environment, we've seen the number of re-renders proliferate to the hundreds. This matches the below reproduction.)

The expected behavior

The lazy parent component should only render one time.


The original issue had a reply that included a reproduction that matched the issue we're experiencing more closely: https://codesandbox.io/p/sandbox/react-lazy-suspense-5g75x3

Similar to what we're seeing, this repro has hundreds of re-renders, and the number is nondeterministic.

Screenshot 2024-05-23 at 11 06 11 AM Screenshot 2024-05-23 at 11 06 21 AM

Our workaround for now is a lightweight replacement for Suspense using the Transition API to defer its lazy children, for usage in nested lazy contexts:

export const DeferredSuspense: FunctionComponent<SuspenseProps> = (props) => {
  const [isDeferred, setIsDeferred] = useState(true);

  useEffect(() => {
    startTransition(() => setIsDeferred(false));
  }, []);

  if (isDeferred) {
    return props.fallback;
  }

  return <Suspense {...props} />;
};
Aryant01 commented 6 months ago

hey @raymondwang . Can you please elaborate the issue and the expected result a bit more for the better understanding. That'll be really helpfull.

raymondwang commented 6 months ago

@Aryant01 The issue is that a Suspense rendered within another Suspense causes errant rerendering of the component within the first Suspense. The examples in the code sandboxes are more illustrative, but here's a brief example:

function Grandchild() {
  return <p>Hello world!</p>
}

function Child() {
  const LazyGrandchild = React.lazy(() => import('./Grandchild'));

  return (
    <Suspense>
      <LazyGrandChild />
    </Suspense>
  );
}

function App() {
  const LazyChild = React.lazy(() => import('./Child'));

  return (
    <Suspense>
      <LazyChild />
    </Suspense>
  );
}

Here's what I'd expect to happen:

  1. App renders
  2. App lazily imports Child
  3. Child renders
  4. Child lazily imports Grandchild
  5. Grandchild renders

Instead, here's what's actually happening:

  1. App renders
  2. App lazily imports Child
  3. Child renders
  4. Child lazily imports Grandchild
  5. Child rerenders
  6. Grandchild renders

I think that what's happening under the hood in step 5 matches what's described here: https://react.dev/reference/react/Suspense#preventing-already-revealed-content-from-hiding — because Child sees a suspended component, its suspense is hoisted up to the closest parent suspense in App.

This issue can balloon far out of control very easily, as seen in this reproduction: https://codesandbox.io/p/sandbox/react-lazy-suspense-5g75x3

Instead of rerendering just once (which might be acceptable), this interaction can cause hundreds of errant rerenders.

alvaro-cuesta commented 6 months ago

I'm still working on a minimum reproduction but I think I'm seeing this. In my case this is also causing error 421 when hydrating after SSR. Removing either the inner lazy component, or just its parent boundary (but leaving the grandparent boundary in) still exhibits error 421.

Anyone else seeing the same or am I chasing a red herring?

gaearon commented 3 months ago

The problem in https://github.com/facebook/react/issues/29235#issuecomment-2127849305 is that the lazy component is declared inside of another component. That’s not supported. You need to move any lazy calls to the top level of the module.

michaelfaith commented 3 months ago

The problem in #29235 (comment) is that the lazy component is declared inside of another component. That’s not supported. You need to move any lazy calls to the top level of the module.

@gaearon in our case, we're caching the lazy component to avoid that issue, but unfortunately still seeing the excessive re-rendering similar to the first codesandbox.

gaearon commented 3 months ago

How are you caching it?

gaearon commented 3 months ago

(Caching it in state won't work for the same reason — it needs to be rooted in something that survives beyond the component's lifetime.)

michaelfaith commented 3 months ago

We have a custom hook that uses a ref to stash the resolved component. So, if ref.current then return that, otherwise, lazy a new instance and set it to ref.current.

gaearon commented 3 months ago

Right, that's essentially the same as storing it in state. It's tied to the lifetime of the component, so it can't survive across attempts to mount it for the first time. Is there a reason to not keep a top-level cache? For lazy that generally makes more sense.

michaelfaith commented 3 months ago

Is there a reason to not keep a top-level cache? For lazy that generally makes more sense.

There's not. This is a design we landed on a while back (pre-concurrent rendering). And it's worked for a while, but it sounds like we need to rework how we're caching these.

gaearon commented 3 months ago

Looking at the OP examples more closely:

https://codesandbox.io/p/sandbox/react-lazy-rerender-bug-forked-q4q6ym

In this original example, there's an extra re-render. That's expected behavior:

  1. First we try to render parent. It suspends. We throw away the tree.
  2. Then we retry. We render parent and child. It suspends. We throw away the tree.
  3. Then we retry. The tree succeeds this time.

So there's an extra parent render but it's expected because we throw away incomplete trees until we're able to mount something. (In 17 and earlier, you'd get an inconsistent tree instead which causes a bunch of other issues.)

Looking at the second OP example:

https://codesandbox.io/p/sandbox/react-lazy-suspense-5g75x3

This one is interesting. I'm not fully sure what's going on here but the pattern is suspicious in general (even if there is a bug). It's a problem in general that you're keeping a Promise (which you suspend on) in state. These Promises need to be rooted outside the component tree so that they survive remount attempts. At least the initial one. (Once it mounts, you can use state to hold the updated ones — kind of like we do here.)

The thing that still feels strange to me is that the inner Suspense isn't enough to limit the issue to the component below. I.e. I'm a bit surprised that it isn't able to mount App since it's not the one doing the suspending (suspending happens inside <Suspense> under App). This part could be a bug. But in general I think you don't really want to keep something you suspend initially on in state at all.

gaearon commented 3 months ago

There's not. This is a design we landed on a while back (pre-concurrent rendering). And it's worked for a while, but it sounds like we need to rework how we're caching these.

Yeah, I think that would be the move. If you're just storing some modules there, then storing it in a top-level Map in the module scope should work well.

michaelfaith commented 3 months ago

Really appreciate the input.

raymondwang commented 3 months ago

Thanks for the insight @gaearon! I really appreciate it. @michaelfaith & I will take a stab at implementing module-level caching for our lazy components.

The thing that still feels strange to me is that the inner Suspense isn't enough to limit the issue to the component below.

This is the part that I'm hung up on, too. It doesn't make intuitive sense to me that nested lazy components would break out of their own Suspense and tear down their parents' trees. The fact that we're able to work around this with the Transition API (with the DeferredSuspense component in the OP) feels like a smell to me, too, since it's an odd workaround to achieve the expected out-of-the-box behavior for a Suspense.

github-actions[bot] commented 5 days ago

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!