fix(react-essentials): correct frame metrics for useUpdateLoop

codedazur · Sep 15, 2023 · fd893de · fd893de
1 parent 77ff46b
commit fd893de
Show file tree

Hide file tree

Showing 4 changed files with 119 additions and 141 deletions.
diff --git a/.changeset/long-birds-argue.md b/.changeset/long-birds-argue.md
@@ -0,0 +1,5 @@
+---
+"@codedazur/react-essentials": patch
+---
+
+provide correct frame metrics for useUpdateLoop
diff --git a/packages/react-essentials/hooks/useUpdateLoop.test.ts b/packages/react-essentials/hooks/useUpdateLoop.test.ts
@@ -1,69 +1,54 @@
 import { act, renderHook } from "@testing-library/react";
-import {
-  beforeAll,
-  afterEach,
-  afterAll,
-  describe,
-  expect,
-  it,
-  vi,
-} from "vitest";
+import { beforeEach, afterEach, describe, expect, it, vi } from "vitest";
 import { useUpdateLoop } from "./useUpdateLoop";
 
-beforeAll(() => {
+beforeEach(() => {
   vi.useFakeTimers();
 
   vi.spyOn(window, "requestAnimationFrame").mockImplementation(
     (callback: FrameRequestCallback): number => {
       /**
-       * Using a timeout of 0 simulates a display with an infinite refresh rate,
-       * allowing us to test any targetFps option, which would otherwise be
-       * limited by the refresh rate of the display. A timeout is still needed
-       * to ensure that the callback is not called synchronously, which would
-       * cause an infinite loop.
+       * A timeout of `1000 / 50` is used to simulate a display with a 50Hz
+       * refresh rate. This value was chosen because it results in a timeout
+       * that is an integer value, which makes the number of frames easier to
+       * calculate.
        */
-      setTimeout(callback, 0);
-
+      setTimeout(callback, 1000 / 50);
       return Math.random();
     },
   );
-});
 
-afterEach(() => {
-  vi.clearAllTimers();
+  vi.spyOn(window, "cancelAnimationFrame").mockImplementation(
+    (id: number): void => {
+      clearTimeout(id);
+      vi.clearAllTimers();
+    },
+  );
 });
 
-afterAll(() => {
+afterEach(() => {
+  vi.runOnlyPendingTimers();
   vi.useRealTimers();
   vi.restoreAllMocks();
 });
 
 describe("useUpdateLoop", () => {
-  it("should support starting the loop immediately", () => {
+  it("should call the onUpdate callback on each frame", () => {
     const onUpdate = vi.fn();
 
-    renderHook(() =>
-      useUpdateLoop({
-        onUpdate,
-        timeScale: 1,
-        targetFps: 60,
-        immediately: true,
-      }),
-    );
+    renderHook(() => useUpdateLoop({ onUpdate, immediately: true }));
 
     vi.advanceTimersByTime(1000);
 
-    expect(onUpdate).toHaveBeenCalledTimes(60);
+    expect(onUpdate).toHaveBeenCalledTimes(50);
   });
 
-  it("should support starting the loop later", () => {
+  it("should support starting the loop manually", () => {
     const onUpdate = vi.fn();
 
     const { result } = renderHook(() =>
       useUpdateLoop({
         onUpdate,
-        timeScale: 1,
-        targetFps: 60,
         immediately: false,
       }),
     );
@@ -78,190 +63,179 @@ describe("useUpdateLoop", () => {
 
     vi.advanceTimersByTime(500);
 
-    expect(onUpdate).toHaveBeenCalledTimes(30);
+    expect(onUpdate).toHaveBeenCalledTimes(25);
   });
 
-  it("should not call the onUpdate callback when the loop is stopped", () => {
+  it("should call the onUpdate callback with the correct frame data", () => {
     const onUpdate = vi.fn();
 
-    const { result } = renderHook(() =>
-      useUpdateLoop({
-        onUpdate,
-        timeScale: 1,
-        targetFps: 60,
-        immediately: true,
+    renderHook(() => useUpdateLoop({ onUpdate, immediately: true }));
+
+    vi.advanceTimersByTime(1000);
+
+    expect(onUpdate).toHaveBeenLastCalledWith(
+      expect.objectContaining({
+        index: 49,
+        time: 1000,
+        deltaTime: 20,
+        fps: 50,
       }),
     );
+  });
+
+  it("should not call the onUpdate callback when the loop is paused", () => {
+    const onUpdate = vi.fn();
+
+    const { result } = renderHook(() =>
+      useUpdateLoop({ onUpdate, immediately: true }),
+    );
 
     vi.advanceTimersByTime(500);
 
-    expect(onUpdate).toHaveBeenCalledTimes(30);
+    expect(onUpdate).toHaveBeenCalledTimes(25);
 
     act(() => {
-      result.current.stop();
+      result.current.pause();
     });
 
     vi.advanceTimersByTime(500);
 
-    expect(onUpdate).toHaveBeenCalledTimes(30);
+    expect(onUpdate).toHaveBeenCalledTimes(25);
   });
 
-  it("should not call the onUpdate callback when the loop is paused", () => {
+  it("should return the correct value for isUpdating", () => {
     const onUpdate = vi.fn();
 
     const { result } = renderHook(() =>
-      useUpdateLoop({
-        onUpdate,
-        timeScale: 1,
-        targetFps: 60,
-        immediately: true,
-      }),
+      useUpdateLoop({ onUpdate, immediately: false }),
     );
 
-    vi.advanceTimersByTime(500);
+    expect(result.current.isUpdating).toBe(false);
 
-    expect(onUpdate).toHaveBeenCalledTimes(30);
+    act(() => {
+      result.current.start();
+    });
+
+    expect(result.current.isUpdating).toBe(true);
 
     act(() => {
       result.current.pause();
     });
 
-    vi.advanceTimersByTime(500);
-
-    expect(onUpdate).toHaveBeenCalledTimes(30);
-  });
+    expect(result.current.isUpdating).toBe(false);
 
-  it("should call the onUpdate callback on each frame", () => {
-    const onUpdate = vi.fn();
+    act(() => {
+      result.current.start();
+    });
 
-    renderHook(() =>
-      useUpdateLoop({
-        onUpdate,
-        timeScale: 1,
-        targetFps: 60,
-        immediately: true,
-      }),
-    );
+    expect(result.current.isUpdating).toBe(true);
 
-    vi.advanceTimersByTime(1000);
+    act(() => {
+      result.current.stop();
+    });
 
-    expect(onUpdate).toHaveBeenCalledTimes(60);
+    expect(result.current.isUpdating).toBe(false);
   });
 
-  it("should call the onUpdate callback with the correct frame data", () => {
+  it("should not call the onUpdate callback when the loop is stopped", () => {
     const onUpdate = vi.fn();
 
-    renderHook(() =>
-      useUpdateLoop({
-        onUpdate,
-        timeScale: 1,
-        targetFps: 60,
-        immediately: true,
-      }),
+    const { result } = renderHook(() =>
+      useUpdateLoop({ onUpdate, immediately: true }),
     );
 
-    vi.advanceTimersByTime(1000);
+    vi.advanceTimersByTime(500);
 
-    expect(onUpdate).toHaveBeenLastCalledWith(
-      expect.objectContaining({
-        index: 59,
-        time: 1000,
-        deltaTime: 16.666666666666668,
-        fps: 60,
-      }),
-    );
+    expect(onUpdate).toHaveBeenCalledTimes(25);
+
+    act(() => {
+      result.current.stop();
+    });
+
+    vi.advanceTimersByTime(500);
+
+    expect(onUpdate).toHaveBeenCalledTimes(25);
   });
 
   it("should reset the frame count when the loop is restarted", () => {
     const onUpdate = vi.fn();
 
     const { result } = renderHook(() =>
-      useUpdateLoop({
-        onUpdate,
-        timeScale: 1,
-        targetFps: 60,
-        immediately: true,
-      }),
+      useUpdateLoop({ onUpdate, immediately: true }),
     );
 
     vi.advanceTimersByTime(500);
 
-    expect(onUpdate).toHaveBeenCalledTimes(30);
+    expect(onUpdate).toHaveBeenCalledTimes(25);
 
     act(() => {
       result.current.stop();
     });
 
-    vi.advanceTimersByTime(500);
-
-    expect(onUpdate).toHaveBeenCalledTimes(30);
-
     act(() => {
       result.current.start();
     });
 
     vi.advanceTimersByTime(500);
 
-    expect(onUpdate).toHaveBeenCalledTimes(60);
+    expect(onUpdate).toHaveBeenCalledTimes(50);
 
     expect(onUpdate).toHaveBeenLastCalledWith(
       expect.objectContaining({
-        index: 59,
-        time: 1000,
-        deltaTime: 16.666666666666668,
-        fps: 60,
+        index: 24,
+        time: 500,
+        deltaTime: 20,
+        fps: 50,
       }),
     );
   });
 
-  it("should take the timescale into account for the time and deltaTime, without affecting the number of frames and callback calls", () => {
+  it("should support a custom timeScale", () => {
     const onUpdate = vi.fn();
 
     renderHook(() =>
       useUpdateLoop({
         onUpdate,
         timeScale: 2,
-        targetFps: 60,
         immediately: true,
       }),
     );
 
     vi.advanceTimersByTime(1000);
 
-    expect(onUpdate).toHaveBeenCalledTimes(60);
+    expect(onUpdate).toHaveBeenCalledTimes(50);
 
     expect(onUpdate).toHaveBeenLastCalledWith(
       expect.objectContaining({
-        index: 59,
+        index: 49,
         time: 2000,
-        deltaTime: 33.333333333333336,
-        fps: 60,
+        deltaTime: 40,
+        fps: 50,
       }),
     );
   });
 
-  it("should take the targetFps into account for the fps and callback calls, without affecting the time and deltaTime", () => {
+  it("should support a targetFps", () => {
     const onUpdate = vi.fn();
 
     renderHook(() =>
       useUpdateLoop({
         onUpdate,
-        timeScale: 1,
-        targetFps: 30,
+        targetFps: 25,
         immediately: true,
       }),
     );
 
     vi.advanceTimersByTime(1000);
 
-    expect(onUpdate).toHaveBeenCalledTimes(30);
+    expect(onUpdate).toHaveBeenCalledTimes(25);
 
     expect(onUpdate).toHaveBeenLastCalledWith(
       expect.objectContaining({
-        index: 29,
-        time: 1000,
-        deltaTime: 16.666666666666668,
-        fps: 30,
+        index: 24,
+        time: 980, // Because the last step does not result in an update.
+        deltaTime: 40,
+        fps: 25,
       }),
     );
   });
@@ -274,8 +248,6 @@ describe("useUpdateLoop", () => {
       ({ onUpdate }) =>
         useUpdateLoop({
           onUpdate,
-          timeScale: 1,
-          targetFps: 60,
           immediately: true,
         }),
       {
@@ -285,13 +257,13 @@ describe("useUpdateLoop", () => {
 
     vi.advanceTimersByTime(500);
 
-    expect(onUpdate1).toHaveBeenCalledTimes(60);
+    expect(onUpdate1).toHaveBeenCalledTimes(25);
 
     rerender({ onUpdate: onUpdate2 });
 
     vi.advanceTimersByTime(500);
 
-    expect(onUpdate1).toHaveBeenCalledTimes(30);
-    expect(onUpdate2).toHaveBeenCalledTimes(30);
+    expect(onUpdate1).toHaveBeenCalledTimes(25);
+    expect(onUpdate2).toHaveBeenCalledTimes(25);
   });
 });