Bug: useInsertionEffect() cleanup function does not fire if a component is wrapped in React.lazy

[JonnyBurger]

React version: 18.2.0

Reproduction

import { lazy, useState, useInsertionEffect, Suspense } from "react";

export default function App() {
  const [show, setShow] = useState(false);

  return (
    <div>
      <Suspense>{show ? <LazyComp2 /> : <LazyComp1 />}</Suspense>
      <button onClick={() => setShow((s) => !s)}>Toggle</button>
    </div>
  );
}

const MyComp = () => {
  useInsertionEffect(() => {
    console.log("start1");
    return () => {
      console.log("stop1");
    };
  }, []);
};

const MyComp2 = () => {
  useInsertionEffect(() => {
    console.log("start2");
    return () => {
      console.log("stop2");
    };
  }, []);
};

const LazyComp1 = lazy(() => Promise.resolve({ default: MyComp }));
const LazyComp2 = lazy(() => Promise.resolve({ default: MyComp2 }));

Click the button twice.

Link to code example:

https://codesandbox.io/s/focused-burnell-ewgso1?file=/src/App.js:0-773

The current behavior

Logs:

start1
start2
stop2
start1

The expected behavior

start1
stop1
start2
stop2
start1

This is also the behavior of useLayoutEffect if not in strict mode.

[Andarist]

I was just about to report the same issue (my repro case can be found here, it's prepared based on @jmca's repro for the issue caused by this - but it's essentially the same repro as yours).

I think that useInsertionEffect is essentially the same as useLayoutEffect, it's just that they have a "higher priority" and they are executed first, in a batch, before useLayoutEffects. So I'm heavily leaning towards your conclusion - this is a bug in React. All other semantics of layout effects should be shared between useInsertionEffect and useLayoutEffect (such as semantics in suspense boundaries, mounting/unmounting them appropriately in Offscreen components, their reappearing and more)

[gaearon]

Seems like a bug. Anyone wants to look into fixing it?

[Andarist]

@gaearon yes! I actually started to work on a PR locally - it's very draft-ish and I only fixed one of the discrepancies so far. Gonna open a draft PR later today.

[Andarist]

To rephrase what the observed issue here is:

1. insertion effects are not disconnected when the suspended subtree gets hidden
2. they are also not disconnected when a different subtree gets finally rendered (when Suspense resumes~)
3. to get them to disconnect we need to "revisit" the old component and switch away from it without suspending.
4. this doesn't match how any other type of effects work. I'd expect those effects to get disconnected in the similar vein as layout effects are but if that's not desirable for some reason then they likely should be disconnected just like passive effects (or using some different semantics of their own but the bottom line is that they should be disconnected at some point)

[sebmarkbage]

This is intentional to avoid layout thrash when things are hidden and restored. It's also to allow for hidden trees to be able to do early preemptive layout calculations for hidden trees so that they can be instantly restored.

[Andarist]

Hmm, but that cleanup has to be called at some point in time - when that is?

[sebmarkbage]

When the Suspense boundary itself is committed and deleted or if an update changes it to no longer be visible. I could imagine the second case being a bug since it’s a thing we typically don’t model at all.

[lubieowoce]

When the Suspense boundary itself is committed and deleted

unless i'm misunderstanding, this doesn't seem to be the case
https://stackblitz.com/edit/react-ts-3vk942?file=App.tsx,Page2.tsx,Page1.tsx

Repro: press "Go to Page 1", "Go to Page 2", "Just unmount the whole thing". the suspense boundary and the suspending children below all get destroyed, but the background stays pink (i.e. Page1's useInsertionEffect is not actually cleaned up)

Interestingly, this only acts "strange" when Page1 or Page2 suspend, not when they're already available. reproducible like this:

• Initial page
• Go to Page 1 (suspends, then makes the background pink)
• Go to Page 2 (suspends, Page1's insertion isn't cleaned up = bg stays pink)
• Go back to Page 1 (doesn't suspend anymore, runs insertion effect)
• Go to Page 2 (doesn't suspend anymore, Page1's insertion effect is cleaned up this time = bg restored to white)

maybe i'm missing something, but i'd expect insertion cleanups to behave the same regardless of whether something suspensey happened (i.e Page2 suspended, Page1 got hidden, and never came back) or not.

[lpolito]

Was there any movement on this? I ran into the same issue linked by @Andarist , specifically with Emotion's Global component not calling the clean up on unmount.

In my case all components are wrapped inside of React.lazy() because that's how we have our page route components set up (all pages being lazily loaded).

[guillaumebrunerie]

For what it's worth, styled-components worked around this bug by simply using useLayoutEffect instead of useInsertionEffect. Maybe Emotion could do the same in the mean time?

[fsjsd]

Any movement on this one?

[owenashurst]

I had to downgrade to React 17.0.2 to resolve this for now.

[Andarist]

@rickhanlonii I somewhat doubt that a PR to Relay fixed this issue 😅

EDIT:// Oh, this might be a byproduct of some of your internal infra. There are some changes in React related to this too: d3d4d3a

[rickhanlonii]

@Andarist yeah the React PR was #30954