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

useTransition: After startTransition, it does not react to passed props changes #17332

Closed
naruaway opened this issue Nov 10, 2019 · 7 comments
Closed

Comments

@naruaway
Copy link

naruaway commented Nov 10, 2019

Do you want to request a feature or report a bug?
Probably a bug
What is the current behavior?
After firing startTransition, "current" component stops reacting to passed props changes while reacting to local state changes.
If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

In the following CodeSandBox, count is counting up in the parent component using setInterval but if we click "CLICK ME", it suddenly stops updating.
https://codesandbox.io/s/usetransition-stop-reacting-passed-props-updates-p9k1b

What is the expected behavior?
When passed props change, it should show the latest value of it where possible

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Experimental build (0.0.0-experimental-5faf377df)

@gaearon
Copy link
Member

gaearon commented Nov 10, 2019

I think this is because setState outside of events is treated as a “normal” priority by default because they’re not caused by a user action. And updates on that priority are batched with the already Suspended updates.

We don’t really expose or document priorities yet but you could bump it to a synchronous update like this:

    const t = setInterval(() => {
      ReactDOM.flushSync(() => {
        setS(x => x + 1);
      });
    }, 200);

This would force it to flush immediately.

I think in this example it leads to some flickering. Could be a bug.

Let’s keep it open both for API discussion and the possible bug.

@naruaway
Copy link
Author

naruaway commented Nov 10, 2019

@cloudever I think that's not actually true. Still high priority updates will cause DOM mutation and I think it's important intended behavior (UI should be always responsive to user input). I created https://codesandbox.io/s/usetransition-stop-reacting-passed-props-updates-8eyke to demonstrate this, where clicking buttons cause numClicks updates correctly.

@gaearon
Thanks, ReactDOM.flushSync is interesting.
FWIW, I noticed one more thing in my CodeSandBox. When using longer timeoutMs like the following,

const [startTransition, isPending] = useTransition({ timeoutMs: 10000 });

count will continue incrementing after button clicks... I am confused.
It looks like the value of timeoutMs is affecting update priority?

@stale
Copy link

stale bot commented Feb 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Feb 9, 2020
@gaearon
Copy link
Member

gaearon commented Feb 9, 2020

Bump because we might want to look again

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Feb 9, 2020
@gaearon
Copy link
Member

gaearon commented Apr 6, 2020

Hmm. It is indeed interesting that timeoutMs: 5000 "gets stuck" but timeoutMs: 10000 keeps ticking. I think this has to do with our batching heuristics. I wonder if the upcoming refactor would solve this or if it's inherent.

@gaearon
Copy link
Member

gaearon commented Apr 6, 2020

Yeah this looks related to the current batching heuristics.

Long story short, we can't know what caused us to suspend. So we have to group updates together and then hope that some combination of them would give us a render that doesn't suspend and gives us something we can commit sooner. Currently, the grouping is based on priority order, so transitions longer than 5 seconds get lumped into the "low priority" group and don't block the normal priority updates. Transitions shorter get batched with normal priority updates, which causes them to be grouped together with your setStates (and thus not letting them commit). We plan to improve these heuristics and make them more granular.

So some version of this problem is inherent, but we can likely improve this case.

@gaearon
Copy link
Member

gaearon commented Jun 29, 2020

This has been fixed on master.
https://codesandbox.io/s/usetransition-stop-reacting-passed-props-updates-zoqm2

@gaearon gaearon closed this as completed Jun 29, 2020
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

2 participants