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: useEffect fires synchronously even though it is not the result of a discrete input #31567

Open
theKastle opened this issue Nov 17, 2024 · 5 comments

Comments

@theKastle
Copy link

useEffect fires synchronously and blocks painting new UI to screen even though it is not the result of a discrete input.

React version: 18.3.1

Steps To Reproduce

  1. setState inside a setTimeout
  2. inside useEffect callback wait for 10_000ms
import { useState, useEffect } from "react";

const sleep = (time) => {
  const wakeUpTime = Date.now() + time;
  while (Date.now() < wakeUpTime) {}
  console.log(time + "ms passed");
};

export default function App() {
  const [text, setText] = useState("Hello");

  useEffect(() => {
    setTimeout(() => {
      setText("Bonjour");
    }, 1000);
  }, []);

  useEffect(() => {
    if (text === "Bonjour") {
     // "Bonjour" text will have to wait for 10000 ms to be shown on screen 
      sleep(10000);
    }
  }, [text]);

  return (
    <div className="App">
      <h1>{text}</h1>
    </div>
  );
}

Link to code example:
https://codesandbox.io/p/sandbox/7zqlmm

The current behavior

After setText("Bonjour") from a setTimeout, it has to wait for 10000ms to be shown on screen.

The expected behavior

After setText("Bonjour") from a setTimeout, it's shown immediately on screen, as the doc says React only flush synchronously useEffect resulting from discrete events like clicks.

Reference:

I don't know if this is a bug or an undocumented behavior. How is the setState inside setTimeout a discrete input ?

@theKastle theKastle added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Nov 17, 2024
@kanakkholwal
Copy link

I think you should wrap this part in asynchronous function and call that function sychronously because current code is blocking the rendering (common practices). Also, Haven't you seen enough codebase where sleep function always is asychronous too?

Current code

const sleep = (time) => {
  const wakeUpTime = Date.now() + time;
  while (Date.now() < wakeUpTime) {}
  console.log(time + "ms passed");
};
// and
useEffect(() => {
    if (text === "Bonjour") {
     // "Bonjour" text will have to wait for 10000 ms to be shown on screen 
      sleep(10000);
    }
  }, [text]);
  

Change this to something like this :

const sleep = (time) => new Promise((resolve) => setTimeout(resolve, time));
// and
useEffect(() => {
  const handleBonjourEffect = async () => {
    if (text === "Bonjour") {
      await sleep(10000); // Allow rendering before this delay
      console.log("10,000ms passed");
    }
  };
  handleBonjourEffect();
}, [text]);

@theKastle
Copy link
Author

theKastle commented Nov 18, 2024

That's not the point 🙂 . The sleep function is just a simulation representing some heavy task.

If you use React 17, you can usually see the text "Bonjour" appear right after setText("Bonjour") (although not always).
https://vcxwdy.csb.app/

Screen.Recording.2024-11-19.at.03.31.56.mov

It's related to this:
#25849
#24982
#25885

The example with React 18 happens because the effect is fired synchronously before layout and paint (before the browser paints the updated screen). From React doc:

If your Effect is caused by an interaction (like a click), React may run your Effect before the browser paints the updated screen.

Additionally, starting in React 18, the function passed to useEffect will fire synchronously before layout and paint when it’s the result of a discrete user input such as a click, or when it’s the result of an update wrapped in flushSync.

The point is why a setState inside a setTimeout (not from an interaction like a click) also causing the effect to run synchronously before paint like in the example?

@kanakkholwal
Copy link

It is firing synchronously because it is synchronous function?

@eps1lon eps1lon added Type: Bug Component: Reconciler and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Nov 21, 2024
@eps1lon
Copy link
Collaborator

eps1lon commented Nov 21, 2024

Thank you for the report.

With React 19, we'll consistently treat updates by default as sync. So unless they're triggered by a non-discrete user input (e.g. mousemove) or are wrapped in startTransition, they're treated as sync and therefore the passive effects are flushed synchronously.

However, there's currently a bug where we don't yield to the browser before flushing passive effects from a concurrent render. That's why wrapping the update in startTransition wouldn't help either at the moment. This could be fixed by treating the passive effect flush from a concurrent render as a "continuation task" instead of a separate scheduler task. Our internal scheduler is set up to support this but it may not be trivial to wire this up correctly. Still, an interesting issue to start digging into reconciler code.

@aalmanasir
Copy link

Bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants