tick immediately when dependencies change

#1464

Author: bmingles

packages/react-hooks/package.json

@@ -23,6 +23,7 @@
   "dependencies": {
     "@deephaven/log": "file:../log",
     "@deephaven/utils": "file:../utils",
+    "lodash.clamp": "^4.0.3",
     "shortid": "^2.2.16"
   },
   "peerDependencies": {

packages/react-hooks/src/useAsyncInterval.test.ts

@@ -2,6 +2,8 @@ import { renderHook, act } from '@testing-library/react-hooks';
 import { TestUtils } from '@deephaven/utils';
 import useAsyncInterval from './useAsyncInterval';
 
+const { asMock } = TestUtils;
+
 beforeEach(() => {
   jest.clearAllMocks();
   expect.hasAssertions();
@@ -15,31 +17,43 @@ afterAll(() => {
 
 describe('useAsyncInterval', () => {
   function createCallback(ms: number) {
-    return jest.fn(
-      async (): Promise<void> =>
-        new Promise(resolve => {
-          setTimeout(resolve, ms);
-        })
-    );
+    return jest
+      .fn(
+        async (): Promise<void> =>
+          new Promise(resolve => {
+            setTimeout(resolve, ms);
+
+            // Don't track the above call to `setTimeout`
+            asMock(setTimeout).mock.calls.pop();
+          })
+      )
+      .mockName('callback');
   }
 
   const targetIntervalMs = 1000;
 
-  it('should call the callback function after the target interval', async () => {
-    const callback = createCallback(50);
-
-    renderHook(() => useAsyncInterval(callback, targetIntervalMs));
+  it('should call the callback function immediately any time the callback or target interval changes', () => {
+    const callbackA = createCallback(50);
+    const callbackB = createCallback(50);
 
-    // First tick should be scheduled for target interval
-    expect(callback).not.toHaveBeenCalled();
-    expect(window.setTimeout).toHaveBeenCalledWith(
-      expect.any(Function),
-      targetIntervalMs
+    const { rerender } = renderHook(
+      ([cb, target]: [() => Promise<void>, number]) =>
+        useAsyncInterval(cb, target),
+      {
+        initialProps: [callbackA, targetIntervalMs],
+      }
     );
 
-    // Callback should be called after target interval
-    act(() => jest.advanceTimersByTime(targetIntervalMs));
-    expect(callback).toHaveBeenCalledTimes(1);
+    expect(callbackA).toHaveBeenCalledTimes(1);
+    jest.clearAllMocks();
+
+    rerender([callbackB, targetIntervalMs]);
+    expect(callbackA).not.toHaveBeenCalled();
+    expect(callbackB).toHaveBeenCalledTimes(1);
+    jest.clearAllMocks();
+
+    rerender([callbackB, targetIntervalMs + 20]);
+    expect(callbackB).toHaveBeenCalledTimes(1);
   });
 
   it('should adjust the target interval based on how long async call takes', async () => {
@@ -48,11 +62,8 @@ describe('useAsyncInterval', () => {
 
     renderHook(() => useAsyncInterval(callback, targetIntervalMs));
 
-    // Callback should be called after target interval
-    expect(callback).not.toHaveBeenCalled();
-    act(() => jest.advanceTimersByTime(targetIntervalMs));
     expect(callback).toHaveBeenCalledTimes(1);
-
+    expect(window.setTimeout).not.toHaveBeenCalled();
     jest.clearAllMocks();
 
     // Mimick the callback Promise resolving
@@ -79,11 +90,7 @@ describe('useAsyncInterval', () => {
 
     renderHook(() => useAsyncInterval(callback, targetIntervalMs));
 
-    // Callback should be called after target interval
-    expect(callback).not.toHaveBeenCalled();
-    act(() => jest.advanceTimersByTime(targetIntervalMs));
     expect(callback).toHaveBeenCalledTimes(1);
-
     jest.clearAllMocks();
 
     // Mimick the callback Promise resolving
@@ -99,12 +106,22 @@ describe('useAsyncInterval', () => {
   });
 
   it('should stop calling the callback function after unmounting', async () => {
-    const callback = createCallback(50);
+    const callbackDelayMs = 50;
+    const callback = createCallback(callbackDelayMs);
 
     const { unmount } = renderHook(() =>
       useAsyncInterval(callback, targetIntervalMs)
     );
 
+    expect(callback).toHaveBeenCalledTimes(1);
+    jest.clearAllMocks();
+
+    // Mimick the callback Promise resolving
+    act(() => jest.advanceTimersByTime(callbackDelayMs));
+    await TestUtils.flushPromises();
+
+    expect(window.setTimeout).toHaveBeenCalledTimes(1);
+
     unmount();
 
     act(() => jest.advanceTimersByTime(targetIntervalMs));
@@ -120,7 +137,6 @@ describe('useAsyncInterval', () => {
       useAsyncInterval(callback, targetIntervalMs)
     );
 
-    act(() => jest.advanceTimersByTime(targetIntervalMs));
     expect(callback).toHaveBeenCalledTimes(1);
     jest.clearAllMocks();
 

packages/react-hooks/src/useAsyncInterval.ts

@@ -1,5 +1,6 @@
 import { useCallback, useEffect, useRef } from 'react';
 import Log from '@deephaven/log';
+import clamp from 'lodash.clamp';
 import { useIsMountedRef } from './useIsMountedRef';
 
 const log = Log.module('useAsyncInterval');
@@ -26,22 +27,31 @@ export function useAsyncInterval(
   targetIntervalMs: number
 ) {
   const isMountedRef = useIsMountedRef();
-  const trackingRef = useRef({ count: 0, started: Date.now() });
   const setTimeoutRef = useRef(0);
+  const trackingRef = useRef({
+    count: 0,
+    started: null as number | null,
+  });
 
   const tick = useCallback(async () => {
     const now = Date.now();
-    let elapsedSinceLastTick = now - trackingRef.current.started;
-
     trackingRef.current.count += 1;
-    trackingRef.current.started = now;
+
+    // If this is our first tick, treat it as if we've already waited the full,
+    // interval, otherwise calculate the elapsed time since the last tick
+    let elapsedSinceLastTick =
+      trackingRef.current.started == null
+        ? targetIntervalMs
+        : now - trackingRef.current.started;
 
     log.debug(
       `tick #${trackingRef.current.count}.`,
       elapsedSinceLastTick,
       'ms elapsed since last tick.'
     );
 
+    trackingRef.current.started = now;
+
     await callback();
 
     if (!isMountedRef.current) {
@@ -50,24 +60,31 @@ export function useAsyncInterval(
 
     elapsedSinceLastTick += Date.now() - trackingRef.current.started;
 
-    // If elapsed time is > than the target interval, adjust the next tick interval
-    const nextTickInterval = Math.max(
+    const overage = clamp(
+      elapsedSinceLastTick - targetIntervalMs,
       0,
-      Math.min(
-        targetIntervalMs,
-        targetIntervalMs - (elapsedSinceLastTick - targetIntervalMs)
-      )
+      targetIntervalMs
     );
 
-    log.debug('adjusted minIntervalMs:', nextTickInterval);
+    const nextTickInterval = targetIntervalMs - overage;
+
+    log.debug(
+      'Next tick target:',
+      targetIntervalMs,
+      ', overage:',
+      overage,
+      ', adjusted:',
+      nextTickInterval
+    );
 
     setTimeoutRef.current = window.setTimeout(tick, nextTickInterval);
   }, [callback, isMountedRef, targetIntervalMs]);
 
   useEffect(() => {
-    log.debug('Setting interval minIntervalMs:', targetIntervalMs);
+    log.debug('Setting target interval:', targetIntervalMs);
 
-    setTimeoutRef.current = window.setTimeout(tick, targetIntervalMs);
+    trackingRef.current.started = null;
+    tick();
 
     return () => {
       window.clearTimeout(setTimeoutRef.current);