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

Should setting state inside discrete events cause cleanup to run? #15020

Closed
gaearon opened this issue Mar 5, 2019 · 3 comments
Closed

Should setting state inside discrete events cause cleanup to run? #15020

gaearon opened this issue Mar 5, 2019 · 3 comments

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Mar 5, 2019

This bug is pretty confusing:

https://twitter.com/kentcdodds/status/1102659818660102145

I think it happens because fn scheduled by setInterval(fn, 0) jumps in front of the [running] effect cleanup caused by setRunning(false). So the interval still fires, overwriting setLapse(0) that happened during the event with its setLapse(someValue).

This reminds me of the issue described in #14750 (comment), or at least a part of it:

In fact, this problem exists even for regular React keystrokes (and other “discrete” events). The solution to that would be to flush passive effects before we get a discrete event.

But here, it seems like this wouldn’t be sufficient because the effect flips as a result of the click, not before it. So should setState inside a discrete event also flush passive effect? Seems like not. (That would defeat the purpose of delaying them.)

So this is working as designed, and the fix is just useLayoutEffect when the timing matters? Or the rAF solution?

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Mar 5, 2019

The problem is that the timer itself isn't a discrete event. Discrete events are only guaranteed in relation to other discrete events. I don't think that's what you want here.

Stopping the timer is an asynchronous operation since all set states are async. So something else can come in before it is flushed.

This is a case where useLayoutEffect doesn't actually fully fix the problem. Certainly not in concurrent mode, but this is also sketchy in sync mode. If it wasn't a timer but let's say a focus event, then that can fire within a batch before this is flushed which would have the same problem.

setRunning(false); // I would like to add a stop of this timer to the queue to be performed later
setLapse(0); // I would like to add an operation to set lapse to zero later
// lots of random stuff that can happen before the batch flushes
// This might also queue an operation to set lapse to something else
// actual rendering
// If concurrent mode, lots of other random stuff that can happen while rendering
// This might also queue an operation to set lapse to something else
// Actually do all that work in order

It's all about what order things are added to the queue.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Mar 5, 2019

One approach is to dispatch the reset only once the timer actually stops, i.e. in the effect. If you want it to always display zero in the same frame, then you can always display zero when running is false.

However, as almost always, this is better modeled as a reducer. The reducer can easily refuse updates to lapse when it thinks it is in a stopped state and also perform the logic to actually reset it.

@gaearon
Copy link
Member Author

@gaearon gaearon commented Mar 8, 2019

@gaearon gaearon closed this Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@sebmarkbage @gaearon and others