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

[React 19] useTransition()'s pending state does not go back to false (revision 94eed63c49-20240425) #28923

Closed
alexeyraspopov opened this issue Apr 26, 2024 · 7 comments · Fixed by #29670

Comments

@alexeyraspopov
Copy link
Contributor

Summary

I am excited to start using React v19 as it has so many features and QoL improvements I've been waiting for!

There is a bug (new bug comparing to v18.2.0) that I found while reproducing #26814. When using useTransition() with use(), pending flag of transition correctly becomes true in the beginning, but doesn't go back to false after transition is complete, which means any pending state artifacts in the UI remain visible.

Repository for reproducing: https://github.com/alexeyraspopov/react-beta-bugs.

Basic code (since the repo contains more than just this bug):

function SimpleAsyncFlow() {
  // this state holds instance of a promise that resolves after a second
  // the same `delayed()` function is used to trigger state update inside SimpleControlledDisplay
  let [value, setValue] = useState(() => delayed(Math.random()));
  return (
    <Suspense fallback={<p>Loading</p>}>
      <SimpleControlledDisplay promise={value} onChange={setValue} />
    </Suspense>
  );
}

function SimpleControlledDisplay({
  promise,
  onChange,
}: {
  promise: Promise<number>;
  onChange: (value: Promise<number>) => void;
}) {
  let value = use(promise);
  let [pending, startTransition] = useTransition();
  let click = () => {
    // this will trigger state update in the parent with the new 1 second promise 
    // that suppose to suspend this component, so transition should prevent it
    startTransition(() => {
      onChange(delayed(Math.random()));
    });
  };
  return (
    <div>
      <button onClick={click}>Refresh</button>
      {/* as UI "pending" state I update text style to become half transparent */}
      <p style={{ opacity: pending ? 0.5 : 1 }}>{value}</p>
    </div>
  );
}

function delayed<T>(value: T) {
  return new Promise<T>((resolve) => setTimeout(resolve, 1000, value));
}

Additionally here's the video reproduction, using the code from the repo I mentioned:

Screen.Recording.2024-04-26.at.08.27.42.mov
@eps1lon
Copy link
Collaborator

eps1lon commented Apr 26, 2024

Does this reproduce in dev and prod?

@alexeyraspopov
Copy link
Contributor Author

@eps1lon I made production build via npx vite build and the behavior remains the same.

@edunomatseye
Copy link

Maybe not a good idea to pass an async value directly or either through a function into state as this behaviour is synchronous. this could result to state inconsistency and also concurrency issues. looking at the delayed function passed into state.

@nakjun12
Copy link
Contributor

Resolving the useTransition Hook Issue: Following Official Documentation

In my opinion, the issue mentioned above is not an actual issue according to the official React documentation. I resolved the problem using the following method:

Solution:

I moved the useTransition hook to the top level of the component, which resolved the error. (https://react.dev/reference/react/useTransition)

Reason:

The official documentation states: "Call useTransition at the top level of your component to mark some state updates as Transitions."

Conclusion:

When using the useTransition hook, it is essential to declare it at the top level of the component.

@eps1lon
Copy link
Collaborator

eps1lon commented May 27, 2024

I moved the useTransition hook to the top level of the component, which resolved the error.

@nakjun12 Can you provide a repro or diff illustrating what you did? From what I can tell, the original repro is valid.

@alexeyraspopov
Copy link
Contributor Author

@eps1lon I did try doing so in my repro code. In SimpleControlledDisplay I swapped two lines of code moving transition hook to the beginning.

function SimpleControlledDisplay({
  promise,
  onChange,
}: {
  promise: Promise<number>;
  onChange: (value: Promise<number>) => void;
}) {
+  let [pending, startTransition] = useTransition();
  let value = use(promise);
-  let [pending, startTransition] = useTransition();

And the transition worked as expected, resetting pending flag after transition is complete. Not sure if this is expected behavior, certainly very confusing one. I will attempt to test this later with RC.

@nakjun12
Copy link
Contributor

@eps1lon
Please refer to the comment above.

acdlite added a commit that referenced this issue May 31, 2024
When a component suspends with `use`, we switch to the "re-render"
dispatcher during the subsequent render attempt, so that we can reuse
the work from the initial attempt. However, once we run out of hooks
from the previous attempt, we should switch back to the regular "update"
dispatcher.

This is conceptually the same fix as the one introduced in
#26232. That fix only accounted
for initial mount, but the useTransition regression test added in
f829733 illustrates that we need to
handle updates, too.

The issue affects more than just useTransition but because most of the
behavior between the "re-render" and "update" dispatchers is the same
it's hard to contrive other scenarios in a test, which is probably why
it took so long for someone to notice.

Closes #28923 and #29209

---------

Co-authored-by: eps1lon <sebastian.silbermann@vercel.com>
github-actions bot pushed a commit that referenced this issue May 31, 2024
When a component suspends with `use`, we switch to the "re-render"
dispatcher during the subsequent render attempt, so that we can reuse
the work from the initial attempt. However, once we run out of hooks
from the previous attempt, we should switch back to the regular "update"
dispatcher.

This is conceptually the same fix as the one introduced in
#26232. That fix only accounted
for initial mount, but the useTransition regression test added in
f829733 illustrates that we need to
handle updates, too.

The issue affects more than just useTransition but because most of the
behavior between the "re-render" and "update" dispatchers is the same
it's hard to contrive other scenarios in a test, which is probably why
it took so long for someone to notice.

Closes #28923 and #29209

---------

Co-authored-by: eps1lon <sebastian.silbermann@vercel.com>

DiffTrain build for [adbec0c](adbec0c)
github-actions bot pushed a commit that referenced this issue May 31, 2024
When a component suspends with `use`, we switch to the "re-render"
dispatcher during the subsequent render attempt, so that we can reuse
the work from the initial attempt. However, once we run out of hooks
from the previous attempt, we should switch back to the regular "update"
dispatcher.

This is conceptually the same fix as the one introduced in
#26232. That fix only accounted
for initial mount, but the useTransition regression test added in
f829733 illustrates that we need to
handle updates, too.

The issue affects more than just useTransition but because most of the
behavior between the "re-render" and "update" dispatchers is the same
it's hard to contrive other scenarios in a test, which is probably why
it took so long for someone to notice.

Closes #28923 and #29209

---------

Co-authored-by: eps1lon <sebastian.silbermann@vercel.com>

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

Successfully merging a pull request may close this issue.

4 participants