useSchedule could produce stray timers even if component was unmounted

[tgohn]

Expectations

If a useSchedule() is not needed anymore, it should not create new timers

Reality

In certain scenarios, we could create a condition where new timers are still being created after useSchedule() are destroyed.

This leads to the following React warning in Lotus:

rollbar.js:5067 Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
    in Dots (at Spinner.js:18)
    in span (at Spinner.js:17)
    in Spinner (at EmptyStateRow.js:61)
    in div (at EmptyStateRow.js:60)
    in td (at EmptyStateRow.js:71)
    in tr (at EmptyStateRow.js:69)
    in EmptyStateRow (created by Context.Consumer)
    in FelaEmptyStateRow (at BasicTable.js:276)

and a lingering requestAnimationFrame loop

Steps to Reproduce

Sample code: https://codesandbox.io/s/competent-thompson-fhtfl?file=/src/App.js

The issue could happen because React setState are called before a setting up new timer.
If the above React setState triggers the cleanup of useLayoutEffect, a new timer will still be created on the next statement.

Perhaps, we can add a new guard variable in the useEffect cleanup callback, and check it before setting up new timers.
Like so:

  useLayoutEffect(() => {
    const performAnimationFrame = () => {
      setTime(Date.now() - start);
      if (!destroyed) tick();
    };

    const onStart = () => {
      loopTimeout = setTimeout(() => {
        cancelAnimationFrame(raf);
        setTime(Date.now() - start);
        if (loop && !destroyed) onStart();
      }, duration);

      // Start the loop
      setDelayComplete(true);
      if (!destroyed) tick();
    };

    const renderingDelayTimeout = setTimeout(onStart, delayMS);

    return () => {
      destroyed = true;
      clearTimeout(renderingDelayTimeout);
      clearTimeout(loopTimeout);
      cancelAnimationFrame(raf);
    };
  }, [duration, delayMS, loop]);

Fine Print

• Container: useSchedule
• Browsers: all

[jzempel]

this was fixed by #196, right?

[austingreendev]

Correct. Closing.