cancelAnimationFrame may not work well in useRaf hook

[hijiangtao]

I think the logic here may not let current animation stop perfectly, which may lead to memory leak or related things.

https://github.com/streamich/react-use/blob/master/src/useRaf.ts#L28-L30

As cancelAnimationFrame method is executed in the clean-up period, the animation ID "stored" in raf can be perfectly cancelled. However, the onFrame method may continues (especially when the main thread render DOM slowly, and it will take sometime between useEffect doing cleaning work and your requestAnimationFrame method's running, https://github.com/streamich/react-use/blob/master/src/useRaf.ts#L9 ), so that a new raf will be assigned, even though you already canvel previous raf.

In this way, you don't clean up previous animation, and next effect will bring in a new animation, which is the problem.

In my opinion, a safe way to guarantee current animation's stop is to add conditions in your main loop method, onFrame method can be rewrited to:

const onFrame = () => {
      const time = Math.min(1, (Date.now() - start) / ms);
      set(time);
      if (YOUR_CANCEL_CONDITION) {
		stop();
	  } else {
        loop();
	  }
};

const stop = () => {
	cancelAnimationFrame(raf);
}

in which YOUR_CANCEL_CONDITION is the logic you should handle with.

[streamich]

I don't really follow. Why would onFrame execute if it was cancelled by cancelAnimationFrame?

[hijiangtao]

In the specification of Promise, which defines cancelAnimationFrame should follow:

cancelAnimationFrame might be called for an entry in the Document’s animation frame request callback list or in the sample all animations operation’s temporary list. In either case the entry’s cancelled flag is set to true so that the callback does not run.

It only guarantees onFrame of selected ID will be stopped, however, In the second parameter, which is the clean-up function, animation ID may has already increased when executing these codes, which is said: the raf you called in cancelAnimationFrame is N, but the above code may already runs loop part and assigned raf with N+1. ID N is stopped, but ID N+1 can still run normally.

The problem is about how react hooks execute codes, I don't know if there is some 'magic' that would change the normal workflow, since I don't read hook code yet.

I use the same code and change your variable assignment to a calculation logic, and got the described problem.

[hijiangtao]

@streamich Sorry for my previous opinion about the problem. I didn't understand the usage of useEffect hook well, and mistakenly blame the problem to the usage of cancelAnimationFrame API.

Thanks for other developers' kindly remind, and I figure out where is the problem. The real reason for the problem of this hook is the wrong usage of effect hooks.

• Another animation frame will be requested before the cleanup function for useEffect is executed.
• Use useLayoutEffect to replace useEffect, in order to guarantee cancelAnimationFrame can work well.

There are two different hooks that you would need to set your eyes on when working with hooks and trying to implement lifecycle functionalities.

Besides useEffect, there is another hook can toggle effect change, and that is useLayoutEffect.

The signature is identical to useEffect, but it fires synchronously after all DOM mutations. Use this to read layout from the DOM and synchronously re-render. Updates scheduled inside useLayoutEffect will be flushed synchronously, before the browser has a chance to paint.

React Hooks API https://reactjs.org/docs/hooks-reference.html#uselayouteffect

Detailed discussion can be found in
https://stackoverflow.com/questions/53781632/whats-useeffect-execution-order-and-its-internal-clean-up-logic-in-react-hooks.

I also raise a PR #77 to fix it. :-)