facebook / react

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

Bug: Conditionally rendering a lazy loaded component only after the parent node is attached causes infinite loop #30582

Open danhorvath opened 1 month ago

danhorvath commented 1 month ago

React version: 18.3.1 and 19.0.0-rc-b57d2823-20240822

Steps To Reproduce

  1. Create a component that renders the children inside a <div>, but only after it has obtained reference to that div (via putting the div node into a state)
  2. Pass a lazy loaded component as children

So basically something like:

import { Suspense, lazy, useState } from 'react';

const LazyLoadedComponent = lazy(
  () =>
    new Promise((resolve) => {
      setTimeout(
        () =>
          resolve({
            default: () => <div>Lazy loaded component</div>,
          }),
        500,
      );
    }),
);

const RenderAfterMount = (props) => {
  const [node, setNode] = useState(null);

  return <div ref={setNode}>{node && props.children}</div>;
};

export const App = () => (
  <Suspense>
    <RenderAfterMount>
      <LazyLoadedComponent />
    </RenderAfterMount>
  </Suspense>
);

Link to code example: https://codesandbox.io/s/vibrant-murdock-jpnznd?file=/src/App.js:542-561

The current behavior

Runtime error due to an infinite loop.

The expected behavior

The lazy loaded component is rendered.

MTtankkeo commented 1 month ago

It may not be a perfect solution, but you need to try the code below I modified.

Modifyed Code

import { Suspense, lazy, useEffect, useRef, useState } from "react";
import "./styles.css";

const LazyLoadedComponent = lazy(
  () =>
    new Promise((resolve) => {
      setTimeout(
        () =>
          resolve({
            default: () => (
              <div style={{ background: "green" }}>Lazy loaded component</div>
            )
          }),
        500
      );
    })
);

export default function App() {
  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <Suspense>
        <RenderAfterMount>
          <LazyLoadedComponent />
        </RenderAfterMount>
      </Suspense>
    </div>
  );
}

const RenderAfterMount = (props) => {
  const [node, setNode] = useState(null);
  const divRef = useRef(null); // This ref references to <div> component.

  // Called when the node state changes.
  useEffect(() => {
    if (divRef.current && !node) {
      setNode(divRef.current);
    }
  }, [node]);

  return <div ref={divRef}>{node && props.children}</div>;
};
MTtankkeo commented 1 month ago

Hey bro, is it still not resolved?, or are you trying?

behnammodi commented 1 month ago

You might need to update RenderAfterMount to:

const RenderAfterMount = (props) => {
  const [node, setNode] = useState(null);

  const popluateNode = (element) => element !== node && setNode(element);

  return <div ref={popluateNode}>{node && props.children}</div>;
};
danhorvath commented 1 month ago

Thanks both, but these workarounds don't solve my issue. In my usecase I need to know about whether the parent element has been attached to the dom. Also, I believe that this is a bug, so i'm not looking for workarounds, I'm hoping to bring attention to it and get it fixed in react.

In short, my usecase is a workaround for https://github.com/facebook/react/issues/23301, where we

  1. first render out our dialog element without any content rendered inside it (and takes the focus)
  2. and then once the dialog element has been attached (and took the focus) we immediately render out it its children that might have autofocus set on them (which would otherwise not be respected inside native dialogs, see the issue in the link).
danhorvath commented 1 month ago

I managed to reproduce the same issue by suspending on a simple promise instead of a lazy loaded component in the render cycle. Here's a sandbox for it: https://codesandbox.io/s/happy-williams-9yjs4p?file=/src/App.js

import { Suspense, useState } from "react";

function wrapPromise(promise) {
  let status = "pending";
  let response;

  const suspender = promise.then(
    (res) => {
      status = "success";
      response = res;
    },
    (error) => {
      status = "error";
      response = error;
    }
  );

  const read = () => {
    switch (status) {
      case "pending": {
        throw suspender;
      }
      case "error": {
        throw response;
      }
      default: {
        return response;
      }
    }
  };

  return { read };
}

const promise = wrapPromise(
  new Promise((resolve) =>
    setTimeout(() => {
      resolve("hello");
    }, 2000)
  )
);

const SuspendableComponent = () => {
  const [divNode, setDivNode] = useState(null);

  // removing the condition unbreaks the render
  if (divNode) {
    console.log(promise.read());
  }

  return <div ref={setDivNode} />;
};

export default function App() {
  return (
    <Suspense fallback={<div>loading...</div>}>
      <SuspendableComponent />
    </Suspense>
  );
}
hungcung2409 commented 1 month ago

Thanks both, but these workarounds don't solve my issue. In my usecase I need to know about whether the parent element has been attached to the dom. Also, I believe that this is a bug, so i'm not looking for workarounds, I'm hoping to bring attention to it and get it fixed in react.

In short, my usecase is a workaround for #23301, where we

  1. first render out our dialog element without any content rendered inside it (and takes the focus)
  2. and then once the dialog element has been attached (and took the focus) we immediately render out it its children that might have autofocus set on them (which would otherwise not be respected inside native dialogs, see the issue in the link).

So it does not need to use setState to accomplish your goal right ?

danhorvath commented 1 month ago

It does need useState. I need a reference to the dialog element and my code needs to react to changes to that ref.

hungcung2409 commented 1 month ago

If I understand correctly, you can use useRef to access to the dialog element.

const RenderAfterMount = (props) => {
  const ref = useRef(null);
  const cb = useCallback((node) => {
    ref.current = node;
    alert("attached");
  }, []);

  return <div ref={cb}>{props.children}</div>;
};

When your lazyComponent mounts, it will call the cb

https://codesandbox.io/p/sandbox/amazing-chaplygin-j5xszv

danhorvath commented 1 month ago

My component needs to re-render when the reference changes, therefore I need a useState. The code example in the bug description is a simplified version of my usecase.