react icon indicating copy to clipboard operation
react copied to clipboard

Bug: state update from a rAF in useLayoutEffect not batched when using `createRoot`

Open mxmul opened this issue 1 year ago • 6 comments

Maybe a weird edge case, but this caused an unexpected visual change in our app when migrating a root from ReactDOM.render.

React version: 18.2.0

Steps To Reproduce

  1. Define a component that has some state, and a useLayoutEffect that modifies the state in a requestAnimationFrame callback. like:
function App() {
  const [message, setMessage] = React.useState("Loading...");

  React.useLayoutEffect(() => {
    requestAnimationFrame(() => {
      setMessage("Ready");
    });
  });
  return <p>{message}</p>;
}
  1. Mount the component using ReactDOM.render, and you will never observe the component with its initial state ("Loading..."). Only the updated state ("Ready").
  2. Mount the component with createRoot and root.render, and you will observe the component render twice. Once with the initial state ("Loading..."), then with the updated state ("Ready").

Link to code example: https://codepen.io/mxmul/pen/abMexLe

react-batching.webm

The current behavior

Two renders - once with the initial state, and a second with the updated state.

The expected behavior

A single render with the updated state. This seems to be the behavior before React 18.

mxmul avatar Feb 27 '24 01:02 mxmul

it is working

import React from "https://esm.sh/[email protected]"; import ReactDOM from "https://esm.sh/[email protected]"; import cx from "https://esm.sh/[email protected]";

function App({ initialMessage }) { const [message, setMessage] = React.useState(initialMessage); // Set initial state to the provided message

React.useEffect(() => { requestAnimationFrame(() => { setMessage("Ready"); }); }, []);

return ( <p className={cx("app", message === "Ready" ? "ready" : "loading")}> {message}

); }

function RemountOnClick({ children }) { const [count, setCount] = React.useState(0);

const handleClick = () => { setCount((n) => n + 1); };

return (

<button type="button" id="render" onClick={handleClick}> Remount
Remount count: {count} {children}
); }

const root = ReactDOM.createRoot(document.getElementById("modern-root")); root.render( <RemountOnClick> <App initialMessage="Ready" /> </RemountOnClick> );

ReactDOM.render( <RemountOnClick> <App initialMessage="Ready" /> </RemountOnClick>, document.getElementById("legacy-root") );

Hariprasadweb avatar Feb 27 '24 11:02 Hariprasadweb

react red flash fix.txt

I have added txt file. please share wheather it is only problem or i misunderstood the problem.

Hariprasadweb avatar Feb 27 '24 11:02 Hariprasadweb

Thanks, but I think this misses the point. This bug report is about React failing to batch a render caused by an immediate state change - your fix only removes the state change.

mxmul avatar Feb 27 '24 16:02 mxmul

A workaround that does work is to wrap the state change with flushSync, like:

React.useLayoutEffect(() => {
  requestAnimationFrame(() => {
    ReactDOM.flushSync(() => {
      setMessage("Ready");
    });
  });
});

But this still seems like a regression to me.

mxmul avatar Feb 27 '24 16:02 mxmul

I checked the code of ReactDOM.render() and ReactDOM.createRoot().render().

Seems like the legacy version always runs flushSync() (seems like modern version does not):

function legacyCreateRootFromDOMContainer(
  container: Container,
  initialChildren: ReactNodeList,
  parentComponent: ?React$Component<any, any>,
  callback: ?Function,
  isHydrationContainer: boolean,
): FiberRoot {
  if (isHydrationContainer) {
    // ...

    flushSync();
    return root;
  } else {
    // First clear any existing content.
    clearContainer(container);

    // ...

    // Initial mount should not be batched.
    flushSync(() => {
      updateContainer(initialChildren, root, parentComponent, callback);
    });

    return root;
  }
}

React will execute flushSync() callback synchronously, applying any updates to the component tree immediately. This ensures that all state and props changes are reflected in the DOM before the method returns.

I guess that's why your workaround solves your issue.

erikasby avatar Mar 15 '24 16:03 erikasby

But I will check a little bit more - it looks like sometimes it also appears instantly as "Ready" on average after ~20 re-renders (for me), seems like worth to investigate this part as well.

erikasby avatar Mar 15 '24 17:03 erikasby

Bundled your example with Vite to check how it is represented there with and without your workaround of flushSync() and really see not much of difference (except the additional 7k lines of .js code 😆). From what I understand:

  • ReactDOM.render() is synchronous and does not support concurrent rendering, e.g. React attempts to render the entire component tree in a single go, which causes the "Loading..." message to briefly appear before the UI with the "Ready" message is rendered.

  • ReactDOM.createRoot().render() enables concurrent rendering, e.g. rendering is split into smaller chunks and updates that are more important for interactivity are prioritized, 2 e.g. React implies that "Loading" is unnecessary to show, because "Ready" is already ready for rendering. (This also explains the occasional "Ready" with ReactDOM.createRoot().render()).

erikasby avatar Mar 16 '24 08:03 erikasby

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[bot] avatar Jun 14 '24 14:06 github-actions[bot]

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[bot] avatar Jun 21 '24 15:06 github-actions[bot]