Skip to content

Commit

Permalink
fix(registration/handAnimation): stop looping after unmount
Browse files Browse the repository at this point in the history
Clearing the timeout is not useful, it will have already fired.
The problem is with the recursive callbacks, which must be broken with a
flag
  • Loading branch information
mnzaki committed Sep 17, 2019
1 parent 2f8f1dc commit 92984eb
Showing 1 changed file with 11 additions and 3 deletions.
14 changes: 11 additions & 3 deletions src/ui/registration/components/handAnimation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ export const usePulseForBoth = () => {
// We need two changing values to use for the opacity,
const handPV = useRef(new Animated.Value(0)).current
const splashPV = useRef(new Animated.Value(0)).current
// NOTE: cannot useState(true) because the value will get captured in the
// pulse callback closure, so we need the indirection of useRef objects
const isPulsingRef = useRef(true)

// here we define how these values change
// in parallel, they change in sync
Expand All @@ -36,14 +39,19 @@ export const usePulseForBoth = () => {
Animated.sequence(
[1, 1, 1, 0, 0, 1, 1].map(n => Animated.timing(handPV, { toValue: n })),
),
]).start(() => {
// this must have itself as a callback to loop
]).start(pulse)
// but only do that if we are stillPulsing
isPulsingRef.current && pulse()
})

// this is react hook magic, whatever functional component this is called in
// will start the loop and clean up when it's lifetime is over
useEffect(() => {
const timeout = setTimeout(pulse, 0)
return () => clearTimeout(timeout)
pulse()
return () => {
isPulsingRef.current = false
}
})

// return your shiny new looping animated values
Expand Down

2 comments on commit 92984eb

@chunningham
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that clearTimeout clears all the memory associated with the callback and so de-allocates the animation stuff, but there's a pretty good chance I'm wrong, and also the flag method is much clearer to understand/read, noice

@mnzaki
Copy link
Contributor Author

@mnzaki mnzaki commented on 92984eb Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clearTimeout would only remove the timeout from the queue, if it hasn't already been executed. And that timeout was just the one that starts the first pulse. The callback which is passed to .start though is used internally and scheduled in a different manner, there are definitely other references to it and wouldn't get garbage collected (and anyway most probably the initial timeout will have executed anyway since the delay was 0 :D)

Please sign in to comment.