feat(raf): add frameloop utils.#824
Conversation
- Added `useFrameloop` util to use unified request animation frame calls. - Added `createScheduledFrameloop` to handle request animation frame from external sources. - Fixed `createRAF` cleanup for id `0` by using `null` instead.
|
| } | ||
|
|
||
| /** | ||
| * Returns an advanced primitive factory function (that has an API similar to `createRAF`) to handle multiple animation frame callbacks in a single batched `requestAnimationFrame`, avoiding the overhead of scheduling multiple animation frames outside of a batch and making them all sync on the same delta. |
There was a problem hiding this comment.
The explanation is way too complex. "A version of createRAF that batches multiple frames within the same render cycle instead of skipping them ."
|
Sorry for the delay on this, the end of the year tends to be really busy. I was starting to re-visit the approach itself and maybe landed into a nicer primitive set, want to hear your opinions on this @atk (I can leave it as an update on this PR, but I don't think this is the right approach for this):
|
atk
left a comment
atk
left a comment
There was a problem hiding this comment.
I don't see my last points addressed, also, the tests seem to fail.
|
Hi @royeden any interest in completing out this PR? We're preparing for Solid Primitives 2.0 and would love to include these improvements. |
📝 WalkthroughWalkthroughAdds three new RAF primitives (createCallbacksSet, useGlobalRAF, createScheduledLoop) to packages/raf, updates createRAF's requestID handling to be nullable, adds new workspace dependencies and tsconfig references, expands test coverage, and documents the new APIs in the README. ChangesRAF Package New Primitives
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant useGlobalRAF
participant SingletonRoot
participant BrowserRAF
Component->>useGlobalRAF: callback, startWhenAdded
useGlobalRAF->>SingletonRoot: add(callback)
SingletonRoot->>SingletonRoot: check startWhenAdded via access()
SingletonRoot->>BrowserRAF: requestAnimationFrame(loop)
BrowserRAF-->>SingletonRoot: timestamp
SingletonRoot->>Component: invoke callback(timestamp)
Component->>useGlobalRAF: remove(callback)
useGlobalRAF->>SingletonRoot: remove from set
SingletonRoot->>BrowserRAF: cancelAnimationFrame (if set empty)
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/raf/test/index.test.ts (1)
274-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame un-awaited
vi.waitUntilissue as above.Line 298's
vi.waitUntil(() => { expect(caf).toHaveBeenCalledTimes(1); })is not awaited and theitcallback (line 280 test, or 274 wrapping) is notasync, so the cancellation-after-dispose assertion never actually gates the test outcome.🐛 Proposed fix
- createRoot(dispose => { + createRoot(async dispose => { ... dispose(); - vi.waitUntil(() => { - expect(caf).toHaveBeenCalledTimes(1); - }); + await vi.waitUntil(() => caf.mock.calls.length >= 1); + expect(caf).toHaveBeenCalledTimes(1); });Note:
createRoot's callback is synchronous by design, so this may require restructuring the test to run outsidecreateRoot(e.g., capturedisposeand await afterwards) for theawaitto work correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raf/test/index.test.ts` around lines 274 - 303, The frameloop dispose test is using vi.waitUntil without awaiting it, so the cancelAnimationFrame assertion never participates in the test result. Update the test around useGlobalRAF/createRoot to make the outer it callback async, capture dispose from createRoot, and await the vi.waitUntil assertion after dispose is called so the caf expectation is properly verified.
🧹 Nitpick comments (2)
packages/raf/src/index.ts (1)
161-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
requestIDis never reset tonullafter cancellation.In
stop(), oncecancel(requestID)runs,requestIDkeeps its stale value. Ifstop()is invoked twice (e.g. manualstop()followed byonCleanup(stop)on dispose),cancel()is called again with the same stale id. This is harmless forcancelAnimationFrame(a browser API tolerant of stale/invalid ids), but sincecancelhere is an arbitrary user-supplied function (e.g. Motion'scancelFrame), redundant calls with a stale/reused reference may not be safe.🛡️ Proposed fix
const stop = () => { setRunning(false); - if (requestID !== null) cancel(requestID); + if (requestID !== null) { + cancel(requestID); + requestID = null; + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raf/src/index.ts` around lines 161 - 172, In the RAF controller’s start/stop logic, the local requestID is left stale after canceling, so a later stop() can cancel the same id again. Update the stop() path in the create RAF helper to clear requestID back to null immediately after calling cancel(requestID), and keep the existing guard in start() so a new schedule(callback) only happens when not already running.packages/raf/test/index.test.ts (1)
42-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAddress the acknowledged test gap.
// TODO add better test — wiring
createScheduledLoopdirectly to rawwindow.requestAnimationFrameonly verifies a single scheduling round-trip, not repeated-loop behavior, which is the primitive's stated purpose. Want me to draft a test using a self-reschedulingschedulefunction (similar tocreateRAF's internalloop) to actually validate repeated invocation and cancellation?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raf/test/index.test.ts` around lines 42 - 63, The current createScheduledLoop test only covers one requestAnimationFrame round-trip, so it does not validate the repeated-loop behavior the primitive is meant to provide. Update the test in createScheduledLoop to use a self-rescheduling scheduler (similar to the loop used by createRAF) and assert that the callback is invoked repeatedly after start() until stop() cancels it. Keep the existing createScheduledLoop API usage and verify both running() state changes and cancelAnimationFrame behavior through the named start, stop, and running helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raf/src/index.ts`:
- Around line 150-177: The Motion adapter in createScheduledLoop is using the
wrong cancellation token, so stop() cannot cancel work scheduled by
frame.render(). Update the schedule/cancel contract to return and store a stable
handle such as the original callback reference from the Motion adapter, then
pass that same handle into cancel from start/stop. Make sure the fix is applied
in createScheduledLoop and the Motion-specific scheduler implementation so the
value returned by schedule is the one later consumed by cancel.
In `@packages/raf/test/index.test.ts`:
- Around line 64-78: The test in createScheduledLoop is using the wrong
primitive and is currently duplicating the createRAF case. Update the test body
to instantiate and exercise createScheduledLoop instead of createRAF, while
keeping the same dispose/cancel assertions so the scheduled loop’s cleanup
behavior is actually covered. Use the createScheduledLoop and createRoot symbols
to locate and correct the copy-paste error.
- Around line 193-253: The automatic-start RAF singleton test is not actually
asserting the async disposal state because vi.waitUntil in the
useGlobalRAF/createRoot block is not awaited. Make the it callback async and
await the vi.waitUntil call so the running() and cancelAnimationFrame
expectations are part of the test result, then keep dispose() after the awaited
check.
---
Duplicate comments:
In `@packages/raf/test/index.test.ts`:
- Around line 274-303: The frameloop dispose test is using vi.waitUntil without
awaiting it, so the cancelAnimationFrame assertion never participates in the
test result. Update the test around useGlobalRAF/createRoot to make the outer it
callback async, capture dispose from createRoot, and await the vi.waitUntil
assertion after dispose is called so the caf expectation is properly verified.
---
Nitpick comments:
In `@packages/raf/src/index.ts`:
- Around line 161-172: In the RAF controller’s start/stop logic, the local
requestID is left stale after canceling, so a later stop() can cancel the same
id again. Update the stop() path in the create RAF helper to clear requestID
back to null immediately after calling cancel(requestID), and keep the existing
guard in start() so a new schedule(callback) only happens when not already
running.
In `@packages/raf/test/index.test.ts`:
- Around line 42-63: The current createScheduledLoop test only covers one
requestAnimationFrame round-trip, so it does not validate the repeated-loop
behavior the primitive is meant to provide. Update the test in
createScheduledLoop to use a self-rescheduling scheduler (similar to the loop
used by createRAF) and assert that the callback is invoked repeatedly after
start() until stop() cancels it. Keep the existing createScheduledLoop API usage
and verify both running() state changes and cancelAnimationFrame behavior
through the named start, stop, and running helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 922a2a45-0592-4713-9037-e7ecdf9780f0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/raf/README.mdpackages/raf/package.jsonpackages/raf/src/index.tspackages/raf/test/index.test.tspackages/raf/tsconfig.json
| function createScheduledLoop< | ||
| RequestID extends NonNullable<unknown>, | ||
| Callback extends (...args: Array<any>) => any, | ||
| >( | ||
| schedule: (callback: Callback) => RequestID, | ||
| cancel: (requestID: RequestID) => void, | ||
| ): (callback: Callback) => [running: Accessor<boolean>, start: VoidFunction, stop: VoidFunction] { | ||
| return (callback: Callback) => { | ||
| if (isServer) { | ||
| return [() => false, noop, noop]; | ||
| } | ||
| const [running, setRunning] = createSignal(false); | ||
| let requestID: RequestID | null = null; | ||
|
|
||
| const start = () => { | ||
| if (running()) return; | ||
| setRunning(true); | ||
| requestID = schedule(callback); | ||
| }; | ||
| const stop = () => { | ||
| setRunning(false); | ||
| if (requestID !== null) cancel(requestID); | ||
| }; | ||
|
|
||
| onCleanup(stop); | ||
| return [running, start, stop]; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What does motion's frame.render() function return, and does cancelFrame require the original callback reference or the return value of frame.render()?
💡 Result:
The frame.render function (part of Motion's frameloop) does not return a value—specifically, it returns undefined [1]. When using cancelFrame, you must provide the original callback reference (the function you passed to frame.render) [1][2][3]. You cannot use a return value, as the function does not provide one [1]. In practice, frame.render is used to schedule a task to run during the render phase of Motion's animation loop [4][5]. Because it is a scheduling utility rather than an execution wrapper with a return handle, cancellation relies entirely on passing the exact function reference that was initially scheduled so it can be identified and removed from the internal queue [1].
Citations:
- 1: https://github.com/motiondivision/motion/blob/cd531784/packages/motion-dom/src/frameloop/render-step.ts
- 2: https://github.com/motiondivision/motion/blob/cd531784/packages/motion-dom/src/frameloop/frame.ts
- 3: https://github.com/motiondivision/motion/blob/cd531784/packages/framer-motion/src/utils/use-animation-frame.ts
- 4: Performance recommendations for scroll motiondivision/motion#3590
- 5: https://motion.dev/docs/frame
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant source and tests.
git ls-files 'packages/raf/src/index.ts' 'test/index.test.ts' 'package.json' 'pnpm-lock.yaml' 'package-lock.json' 'yarn.lock' | sed 's/^/FILE /'
echo
echo '--- packages/raf/src/index.ts (selected range) ---'
sed -n '1,240p' packages/raf/src/index.ts | cat -n
echo
echo '--- test/index.test.ts (selected range) ---'
sed -n '1,140p' test/index.test.ts | cat -n
echo
echo '--- search for Motion usage and createScheduledLoop/createRAF ---'
rg -n "createScheduledLoop|createRAF|frame\.render|cancelFrame|requestAnimationFrame" packages test . -g '!**/dist/**' -g '!**/build/**'Repository: solidjs-community/solid-primitives
Length of output: 11284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- repo files matching raf tests ---'
fd -a 'index.test.ts|raf.*test.*|test.*ts' . packages test
echo
echo '--- package.json ---'
sed -n '1,240p' package.json | cat -n
echo
echo '--- package-specific package.json files ---'
fd -a 'package.json' packages | while read -r f; do
echo "FILE $f"
sed -n '1,220p' "$f" | rg -n '"(motion|`@motionone`|framer-motion|solid-js)"'
done
echo
echo '--- search for createScheduledLoop tests/usages ---'
rg -n "createScheduledLoop|frame\.render|cancelFrame|targetFPS|createRAF" packages . -g '!**/dist/**' -g '!**/build/**'Repository: solidjs-community/solid-primitives
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- packages/raf/test/index.test.ts (relevant section) ---'
sed -n '1,120p' packages/raf/test/index.test.ts | cat -n
echo
echo '--- packages/raf/README.md (createScheduledLoop section) ---'
sed -n '200,245p' packages/raf/README.md | cat -n
echo
echo '--- motion dependency presence in lockfile/package ---'
rg -n '"motion"|from "motion"|from '\''motion'\''|cancelFrame|frame\.render' package.json packages/raf/package.json pnpm-lock.yaml packages/raf/README.md packages/raf/src/index.tsRepository: solidjs-community/solid-primitives
Length of output: 8657
Fix the Motion example’s cancellation token
frame.render() returns undefined, while cancelFrame expects the original callback reference. As written, stop() has nothing usable to pass to cancelFrame, so the scheduled work can’t be canceled. Return the callback (or another stable handle) from the Motion adapter and use that for cancel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raf/src/index.ts` around lines 150 - 177, The Motion adapter in
createScheduledLoop is using the wrong cancellation token, so stop() cannot
cancel work scheduled by frame.render(). Update the schedule/cancel contract to
return and store a stable handle such as the original callback reference from
the Motion adapter, then pass that same handle into cancel from start/stop. Make
sure the fix is applied in createScheduledLoop and the Motion-specific scheduler
implementation so the value returned by schedule is the one later consumed by
cancel.
| it("frameloop created with requestAnimationFrame calls cancelAnimationFrame after dispose", () => { | ||
| const raf = vi.spyOn(window, "requestAnimationFrame"); | ||
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | ||
| createRoot(dispose => { | ||
| const [running, start] = createRAF(ts => { | ||
| expect(typeof ts === "number"); | ||
| }); | ||
| expect(running()).toBe(false); | ||
| expect(raf).not.toHaveBeenCalled(); | ||
| expect(caf).not.toHaveBeenCalled(); | ||
| start(); | ||
| expect(running()).toBe(true); | ||
| expect(raf).toHaveBeenCalled(); | ||
| dispose(); | ||
| expect(caf).toHaveBeenCalled(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrong primitive under test — copy-paste bug.
This test lives in describe("createScheduledLoop") but calls createRAF at line 68, identical to the earlier createRAF test. createScheduledLoop's dispose/cancel behavior is never actually exercised here.
🐛 Proposed fix
it("frameloop created with requestAnimationFrame calls cancelAnimationFrame after dispose", () => {
const raf = vi.spyOn(window, "requestAnimationFrame");
const caf = vi.spyOn(window, "cancelAnimationFrame");
createRoot(dispose => {
- const [running, start] = createRAF(ts => {
+ const [running, start] = createScheduledLoop(
+ window.requestAnimationFrame,
+ window.cancelAnimationFrame,
+ )(ts => {
expect(typeof ts === "number");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("frameloop created with requestAnimationFrame calls cancelAnimationFrame after dispose", () => { | |
| const raf = vi.spyOn(window, "requestAnimationFrame"); | |
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | |
| createRoot(dispose => { | |
| const [running, start] = createRAF(ts => { | |
| expect(typeof ts === "number"); | |
| }); | |
| expect(running()).toBe(false); | |
| expect(raf).not.toHaveBeenCalled(); | |
| expect(caf).not.toHaveBeenCalled(); | |
| start(); | |
| expect(running()).toBe(true); | |
| expect(raf).toHaveBeenCalled(); | |
| dispose(); | |
| expect(caf).toHaveBeenCalled(); | |
| it("frameloop created with requestAnimationFrame calls cancelAnimationFrame after dispose", () => { | |
| const raf = vi.spyOn(window, "requestAnimationFrame"); | |
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | |
| createRoot(dispose => { | |
| const [running, start] = createScheduledLoop( | |
| window.requestAnimationFrame, | |
| window.cancelAnimationFrame, | |
| )(ts => { | |
| expect(typeof ts === "number"); | |
| }); | |
| expect(running()).toBe(false); | |
| expect(raf).not.toHaveBeenCalled(); | |
| expect(caf).not.toHaveBeenCalled(); | |
| start(); | |
| expect(running()).toBe(true); | |
| expect(raf).toHaveBeenCalled(); | |
| dispose(); | |
| expect(caf).toHaveBeenCalled(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raf/test/index.test.ts` around lines 64 - 78, The test in
createScheduledLoop is using the wrong primitive and is currently duplicating
the createRAF case. Update the test body to instantiate and exercise
createScheduledLoop instead of createRAF, while keeping the same dispose/cancel
assertions so the scheduled loop’s cleanup behavior is actually covered. Use the
createScheduledLoop and createRoot symbols to locate and correct the copy-paste
error.
| it("(Automatic start) global RAF singleton calls rafs with the same timestamp", () => { | ||
| // Note on this test: For some reason, the raf is being called twice, once when started (but id doesn't invoke the callback for some strange reason) and once after the timers advance (and the callback is properly invoked). | ||
| const raf = vi.spyOn(window, "requestAnimationFrame"); | ||
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | ||
| createRoot(dispose => { | ||
| const timestamps = new Set<number>(); | ||
| const createGlobalRAFCallback = useGlobalRAF(); | ||
| const callback1: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | ||
| const [added1, add1, remove1, running1, start1, stop1] = createGlobalRAFCallback( | ||
| callback1, | ||
| true, | ||
| ); | ||
| const callback2: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | ||
| const [added2, add2, remove2, running2, start2, stop2] = createGlobalRAFCallback( | ||
| callback2, | ||
| true, | ||
| ); | ||
|
|
||
| // Queue functions should not be equal | ||
| expect(added1).not.toEqual(added2); | ||
| expect(add1).not.toEqual(add2); | ||
| expect(remove1).not.toEqual(remove2); | ||
| expect(callback1).not.toHaveBeenCalled(); | ||
| expect(callback2).not.toHaveBeenCalled(); | ||
|
|
||
| // Frameloop functions should be equal because of the singleton | ||
| expect(running1).toEqual(running2); | ||
| expect(start1).toEqual(start2); | ||
| expect(stop1).toEqual(stop2); | ||
|
|
||
| // Aliases | ||
| const running = running1; | ||
| const stop = stop1; | ||
|
|
||
| expect(added1()).toBe(false); | ||
| add1(); | ||
| expect(added1()).toBe(true); | ||
| expect(added2()).toBe(false); | ||
| expect(running()).toBe(true); | ||
| vi.advanceTimersToNextFrame(); | ||
| expect(raf).toHaveBeenCalledTimes(2); | ||
| expect(callback1).toHaveBeenCalledTimes(1); | ||
| expect(timestamps.size).toEqual(1); | ||
| stop(); | ||
| expect(running()).toBe(false); | ||
| expect(caf).toHaveBeenCalledTimes(1); | ||
| add2(); | ||
| expect(added2()).toBe(true); | ||
| expect(running()).toBe(true); | ||
| vi.advanceTimersToNextFrame(); | ||
| expect(raf).toHaveBeenCalledTimes(4); | ||
| expect(timestamps.size).toEqual(2); | ||
| remove1(); | ||
| remove2(); | ||
| vi.waitUntil(() => { | ||
| expect(running()).toBe(false); | ||
| expect(caf).toHaveBeenCalledTimes(2); | ||
| }); | ||
| dispose(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
vi.waitUntil assertions are never verified — not awaited.
vi.waitUntil returns a Promise and this call is not awaited nor is the enclosing it callback async. The assertions inside its callback (expect(running()).toBe(false), expect(caf).toHaveBeenCalledTimes(2)) will never actually gate the test result — dispose() runs immediately after, and any failures inside the un-awaited promise surface at best as an unhandled rejection, not a test failure. This test currently provides false confidence about disposal/cancellation behavior in the automatic-start scenario.
🐛 Proposed fix
- it("(Automatic start) global RAF singleton calls rafs with the same timestamp", () => {
+ it("(Automatic start) global RAF singleton calls rafs with the same timestamp", async () => {
...
remove1();
remove2();
- vi.waitUntil(() => {
- expect(running()).toBe(false);
- expect(caf).toHaveBeenCalledTimes(2);
- });
+ await vi.waitUntil(() => !running());
+ expect(caf).toHaveBeenCalledTimes(2);
dispose();
- });
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("(Automatic start) global RAF singleton calls rafs with the same timestamp", () => { | |
| // Note on this test: For some reason, the raf is being called twice, once when started (but id doesn't invoke the callback for some strange reason) and once after the timers advance (and the callback is properly invoked). | |
| const raf = vi.spyOn(window, "requestAnimationFrame"); | |
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | |
| createRoot(dispose => { | |
| const timestamps = new Set<number>(); | |
| const createGlobalRAFCallback = useGlobalRAF(); | |
| const callback1: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added1, add1, remove1, running1, start1, stop1] = createGlobalRAFCallback( | |
| callback1, | |
| true, | |
| ); | |
| const callback2: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added2, add2, remove2, running2, start2, stop2] = createGlobalRAFCallback( | |
| callback2, | |
| true, | |
| ); | |
| // Queue functions should not be equal | |
| expect(added1).not.toEqual(added2); | |
| expect(add1).not.toEqual(add2); | |
| expect(remove1).not.toEqual(remove2); | |
| expect(callback1).not.toHaveBeenCalled(); | |
| expect(callback2).not.toHaveBeenCalled(); | |
| // Frameloop functions should be equal because of the singleton | |
| expect(running1).toEqual(running2); | |
| expect(start1).toEqual(start2); | |
| expect(stop1).toEqual(stop2); | |
| // Aliases | |
| const running = running1; | |
| const stop = stop1; | |
| expect(added1()).toBe(false); | |
| add1(); | |
| expect(added1()).toBe(true); | |
| expect(added2()).toBe(false); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(2); | |
| expect(callback1).toHaveBeenCalledTimes(1); | |
| expect(timestamps.size).toEqual(1); | |
| stop(); | |
| expect(running()).toBe(false); | |
| expect(caf).toHaveBeenCalledTimes(1); | |
| add2(); | |
| expect(added2()).toBe(true); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(4); | |
| expect(timestamps.size).toEqual(2); | |
| remove1(); | |
| remove2(); | |
| vi.waitUntil(() => { | |
| expect(running()).toBe(false); | |
| expect(caf).toHaveBeenCalledTimes(2); | |
| }); | |
| dispose(); | |
| }); | |
| }); | |
| it("(Automatic start) global RAF singleton calls rafs with the same timestamp", async () => { | |
| // Note on this test: For some reason, the raf is being called twice, once when started (but id doesn't invoke the callback for some strange reason) and once after the timers advance (and the callback is properly invoked). | |
| const raf = vi.spyOn(window, "requestAnimationFrame"); | |
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | |
| createRoot(dispose => { | |
| const timestamps = new Set<number>(); | |
| const createGlobalRAFCallback = useGlobalRAF(); | |
| const callback1: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added1, add1, remove1, running1, start1, stop1] = createGlobalRAFCallback( | |
| callback1, | |
| true, | |
| ); | |
| const callback2: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added2, add2, remove2, running2, start2, stop2] = createGlobalRAFCallback( | |
| callback2, | |
| true, | |
| ); | |
| // Queue functions should not be equal | |
| expect(added1).not.toEqual(added2); | |
| expect(add1).not.toEqual(add2); | |
| expect(remove1).not.toEqual(remove2); | |
| expect(callback1).not.toHaveBeenCalled(); | |
| expect(callback2).not.toHaveBeenCalled(); | |
| // Frameloop functions should be equal because of the singleton | |
| expect(running1).toEqual(running2); | |
| expect(start1).toEqual(start2); | |
| expect(stop1).toEqual(stop2); | |
| // Aliases | |
| const running = running1; | |
| const stop = stop1; | |
| expect(added1()).toBe(false); | |
| add1(); | |
| expect(added1()).toBe(true); | |
| expect(added2()).toBe(false); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(2); | |
| expect(callback1).toHaveBeenCalledTimes(1); | |
| expect(timestamps.size).toEqual(1); | |
| stop(); | |
| expect(running()).toBe(false); | |
| expect(caf).toHaveBeenCalledTimes(1); | |
| add2(); | |
| expect(added2()).toBe(true); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(4); | |
| expect(timestamps.size).toEqual(2); | |
| remove1(); | |
| remove2(); | |
| await vi.waitUntil(() => !running()); | |
| expect(caf).toHaveBeenCalledTimes(2); | |
| dispose(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raf/test/index.test.ts` around lines 193 - 253, The automatic-start
RAF singleton test is not actually asserting the async disposal state because
vi.waitUntil in the useGlobalRAF/createRoot block is not awaited. Make the it
callback async and await the vi.waitUntil call so the running() and
cancelAnimationFrame expectations are part of the test result, then keep
dispose() after the awaited check.
3 participants
Added
createCallbacksSetutil to unify calling multiple void callbacks stored in aReactiveSet.Added
createScheduledLoopto handle external animation loops, such asmotion's frame util.Added
useGlobalRAFutil to handle unified request animation frame calls.Fixed
createRAFcleanup for id0by usingnullinstead.Addresses this proposal #822.
Summary by CodeRabbit
New Features
Bug Fixes
Tests