Bug: Potential bug in “official” useInterval example
gmoniava opened this issue · 19 comments
Maybe this is not a bug of react, but this is related to a blog post by react team member; many people have probably read or used that post.
The bug is explained here: https://stackoverflow.com/questions/68407187/potential-bug-in-official-useinterval-example
I think feedback from react team would be useful.
The report on Stack Overflow doesn't look like a bug to me.
The answer that suggests updating the ref during render would be a side effect, which should be avoided because it will cause problems.
The demo shows that the current callback value may differ from that in useEffect alright, but the real question is which one of them is the "correct" one?
The "correct" one is the one that has been committed. For one reason, committed effects are the only ones that are guaranteed to have cleanup phase later. (The interval in this question doesn't need a cleanup effect, but other things might.)
Another more compelling reason in this case, perhaps, is that React may pre-render things (either at lower priority, or because they're "offscreen" and not yet visible, or in the future b'c of animation APIs). Pre-rendering work like this should never modify a ref, because the modification would be arbitrary. (Consider a future animation API that pre-renders multiple possible future visual states to make transitions faster in response to user interaction. You wouldn't want the one that happened to render last to just mutate a ref that's used by your currently visible/committed view.)
I'm going to close this issue since it's not a bug, and since it seems like the main thing you were looking for is input. I'll respond on the Stack Overflow.
@bvaughn AFAIK the author doesn't suggest to update the ref in render, he was just contemplating. I pointed him to a github issue which recommended against that and he mentions that. You can see his alternative solution doesn't update the ref in render. Regardless, the useInterval appears to have a bug as described in the question, isn't it? It doesn't fire the latest callback, if that callback was updated in render, and didn't have time to be updated also by useEffect.
I think that thinking of it as an "old" callback is maybe the wrong mental model. At some point in the future, after an application updates, any callback is an "old" callback.
See my edited answer on Stack Overflow.
@bvaughn FYI another developer independently reported similar issue as a bug: https://stackoverflow.com/questions/61999810/store-a-callback-in-useref/68104782#68104782. I understand them, because AFAIK the goal of that approach Dan did in that code, was that, if someone passed a new callback to useInterval during render, then that callback was supposed to be executed instead of a one from previous render.
if someone passed a new callback to
useIntervalduring render, then that callback was supposed to be executed instead of a one from previous render.
The new callback prop should only be executed if it was committed.
The reason Dan suggested using a ref + effect for this was so that the overall interval doesn't get reset whenever the view is updated.
@bvaughn Quote from Dan's blog:
So what if we didn’t replace the interval at all, and instead introduced a mutable savedCallback variable pointing to the latest interval callback?
However, thanks to the savedCallback ref, we can always read the callback that we set after the last render, and call it from the interval tick.
How about this?
How about what? I'm not sure I understand.
When Dan says, "after the last render", he literally means "after the render, in an effect" (as is shown in the code example right below that)
@bvaughn Ok, it seems confusion comes from the fact, which callback do we consider the latest. I am calling latest the one which was updated as a result of latest render (before useEffect was run).
I am also not very familiar with the concept you used above committed, as I haven't started using React 17.
What I am trying to say you can see there are at least three developers who got confused about this. And if someone was using that custom hook useInterval they might be expecting it to use the latest callback (latest in the sense as the one which they passed during last render), isn't it?
I think our conversation is kind of going in a circle.
Originally, you asked if this was a bug and I responded and said it wasn't, and tried to explain why. I also responded to the Stack Overflow as well.
I'm not sure what we're talking about now. It seems like you're concerned that the article is a little confusing, and maybe it is. Maybe it could have been clearer. That's not something I can address though.
@bvaughn Ok, my point is if I was a consumer of useInterval custom hook, I would expect it to run the latest callback always - by latest, I mean the one which I passed during last render, isn't it?
I think you'd have to take this up with Dan. I'm not sure what else to add. The way it's written seems intentional (no side effects during render).
@bvaughn I don't know how to reach him I don't have twitter, maybe you can show him the stack overflow question. At least you can see I am not the only one confused.
There is also a related question: https://stackoverflow.com/questions/61999810/store-a-callback-in-useref, in case you will be interested to write an answer for on Stack Overflow.
I'm pretty sure that the original userInterval code is buggy.
The following is ultimately a summary of all the contributions by others in my SO question.
Render Phase Callbacks
const [size, setSize] = React.useState();
const onInterval = () => {
console.log(size)
}
useInterval(onInterval, 100);onInterval in this example will only be the "correct" callback after the DOM has been committed.
Concurrent mode has it that the render phase resulting in a new value for onInterval may not even make it to the DOM.
Calling savedCallback.current = callback outside an effect constitutes a side-effect (as @bvaughn pointed out), so such assignment must happen within an effect - as in the original useInterval implementation.
A Fraction of Time
I think what @gaearon was missing is that the interval could fire after the DOM has been committed, but before useEffect sets savedCallback.current = callback.
Once the DOM has been committed, the incoming callback argument in useInterval is the one that should be used in the interval callback.
As @bvaughn noted. using useLayoutEffect instead of useEffect would solve this. So the useInterval code should change to:
// Remember the latest callback.
useLayoutEffect(() => {
savedCallback.current = callback;
}, [callback]);I reckon this issue should move to https://github.com/gaearon/overreacted.io/issues and be re-opened there.
Owners of this repo should have an option to "Move issue". Guess if it ain't done in the next 48 hours we should just open a new one there. I don't mind doing it.
Owners of this repo should have an option to "Move issue"
This option only works for other projects within the same org (facebook). The repo you mentioning above is not a part of the facebook org.
Done.