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

add useInterval hook #5

Closed
wants to merge 1 commit into from
Closed

Conversation

escaton
Copy link

@escaton escaton commented Jan 9, 2019

No description provided.

@gragland
Copy link
Contributor

Thanks! I found this other solution you proposed to be pretty easy to understand: facebook/react#14543 (comment)

Are there any downsides to that solution vs. the one in this pull request?

@escaton
Copy link
Author

escaton commented Jan 11, 2019

Let me highlight the problem: we need to execute some code in cycle with constant time interval. Similar to what setInterval does. And that code depends on component state.
So the problem here

const [state, setState] = useState(0)
useEffect(() => {
  const id = setInterval(
    () => { console.log(state) } // always logs "0"
  , 1000)
  return () => clearInterval(id)
}, [])

is that the first state is trapped in interval closure.
Hooks docs told us: if our effect depends on something from outside, this something should be in second argument.

useEffect(() => {
  const id = setInterval(
    () => { console.log(state) } // works almost as expected
  , 1000)
  return () => clearInterval(id)
}, [state])

Looks fine, but now it doesn't meet the criteria of constant time interval between calls. Because every time state changes setInterval restarts.

Looking back there was no such problem before: using class components we have state coupled to instance and this always have reference to actual state value.

componentDidMount() {
  setInterval(
    () => { console.log(this.state) } // "unboxing" state gives actual value every time
  , 1000)
}

So how it could be solved?

The first approach is pretty straight. We need something that will hold the reference to state like this in class components does it.
While there is still component's own this in functional components (except those created with arrow function) React doesn't keep there anything useful for us. Instead we haveuseRef so lets actually use it :)

const [state, setState] = useState()
const stateRef = useRef(state)
useEffect({
  const id = setInterval(
    () => { console.log(stateRef.current) } // still logs "0" every time
  , 1000)
  return () => clearInterval(id)
}, []) // no state here

Now we have to update this reference every time state changes.
And here is the question: is reference mutation a side effect? Honest answer would be "yes".
If so than we need to wrap it in another useEffect:

const stateRef = useRef(state)
useEffect(
  () => stateRef.current = state,
  [state]
)
useEffect({
  const id = setInterval(
    () => { console.log(stateRef.current) } // works nicely
  , 1000)
  return () => clearInterval(id)
}, []) // neither state nor stateRef here

Ok, now we've got working solution. Is everything fine? Almost.
Now we have implicit dependency between two effects. And what's worse: there is a lag between state and stateRef.current changes. That means that we can't use stateRef anywhere but in subsequent effects (depending on effects execution order)

const stateRef = useRef(state)
useEffect(
  () => stateRef.current = state,
  [state]
)
useEffect({
  const id = setInterval(
    () => { console.log(stateRef.current === state) }  // true
  , 1000)
  return () => clearInterval(id)
}, [])
console.log(stateRef.current === state) // true? only until setState(...) will be called anywhere

So there are still issues. What if we suppose that mutating reference outside of effect is... acceptable?
Well it turns out that then we will achieve almost same behavior as in class components.

const stateRef = useRef(state)
stateRef.current = state;
useEffect({
 const id = setInterval(
   () => { console.log(stateRef.current === state) }  // true
 , 1000)
 return () => clearInterval(id)
}, [])
console.log(stateRef.current === state) // obviously always true :)

I could finish here, but i feel like this is not ideal, at least until react team told that this is ok.

And there are at least two more idiomatic solutions :)
The first is in this PR and its about refactoring effect to make it tolerant to restart. No indirect useRef usage, no implicit dependencies on other effects or their execution order. Just swap setInterval with setTimeout and adjust timeouts.

The other possible solution is about turning original problem in the following way: what we actually want is two separate effects. One is timer tick, the other is tick body.

const [tick, setTick] = useState(0)

useEffect(() => {
  const id = setInterval(() => {
    setTick(t => t+1)
  }, 5000)
  return () => clearInterval(id)
}, [])

useEffect(() => {
  console.log(state) // always actual state
}, [tick]) // although state is not here

In my eyes it looks very nice although it causes rerenders every 5 seconds.

What would you prefer? :)

@gragland
Copy link
Contributor

@escaton Thanks for breaking that down for me! I'd be interested to hear from the React team if mutating the reference outside of the effect is acceptable in this case. I sure find it easier to quickly understand, but of course don't want to recommend an anti-pattern. Otherwise the last solution will probably work. Will think on this some more and see if I can get any input from them.

@Yurickh
Copy link

Yurickh commented Feb 6, 2019

Dan wrote a blogpost on this exact topic: https://overreacted.io/making-setinterval-declarative-with-react-hooks/

@gragland
Copy link
Contributor

gragland commented Feb 6, 2019

@Yurickh Thanks! Yeah that settles that then. @escaton Seems really close to what you have. I think I'd like to post it exactly how Dan does it so that I can just link over to his blog post for the full explanation.

@escaton
Copy link
Author

escaton commented Feb 6, 2019

Of course

@gragland
Copy link
Contributor

Closing because explaining how to make setInternal declarative is too lengthy for the format of this blog. I'll leave that to Dan :)

@gragland gragland closed this Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants