From 1adba485c639cae8c1d28aa896010bad1025f579 Mon Sep 17 00:00:00 2001 From: Coronon <33808743+Coronon@users.noreply.github.com> Date: Sat, 2 Nov 2024 02:21:36 +0100 Subject: [PATCH 1/3] fix: useDebounce race condition (#139) --- .../useDebounce/useDebounce.svelte.ts | 71 +++++++++++-------- .../useDebounce/useDebounce.test.svelte.ts | 27 +++++++ 2 files changed, 69 insertions(+), 29 deletions(-) diff --git a/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts b/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts index 404ab2bc..1626bb49 100644 --- a/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts +++ b/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts @@ -8,6 +8,13 @@ type UseDebounceReturn = (( pending: boolean; }; +type DebounceContext = { + timeout: ReturnType | undefined; + resolve: (value: Return) => void; + reject: (reason: unknown) => void; + promise: Promise; +}; + /** * Function that takes a callback, and returns a debounced version of it. * When calling the debounced function, it will wait for the specified time @@ -27,62 +34,68 @@ export function useDebounce( callback: (...args: Args) => Return, wait: MaybeGetter = 250 ): UseDebounceReturn { - let timeout = $state>(); - let resolve: null | ((value: Return) => void) = null; - let reject: null | ((reason: unknown) => void) = null; - let promise: Promise | null = null; - - function reset() { - timeout = undefined; - promise = null; - resolve = null; - reject = null; - } + let context = $state | null>(null); function debounced(this: unknown, ...args: Args) { - if (timeout) { - clearTimeout(timeout); - } - - if (!promise) { - promise = new Promise((res, rej) => { + if (context) { + // Old context will be reused so callers awaiting the promise will get the + // new value + if (context.timeout) { + clearTimeout(context.timeout); + } + } else { + // No old context, create a new one + let resolve: (value: Return) => void; + let reject: (reason: unknown) => void; + let promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + + context = { + timeout: undefined, + promise, + resolve: resolve!, + reject: reject!, + }; } - timeout = setTimeout( + context.timeout = setTimeout( async () => { + // Grab the context and reset it + // -> new debounced calls will create a new context + if (!context) return; + const ctx = context; + context = null; + try { - resolve?.(await callback.apply(this, args)); + ctx.resolve(await callback.apply(this, args)); } catch (error) { - reject?.(error); - } finally { - reset(); + ctx.reject(error); } }, typeof wait === "function" ? wait() : wait ); - return promise; + return context.promise; } debounced.cancel = async () => { - if (timeout === undefined) { + if (!context || !context.timeout) { // Wait one event loop to see if something triggered the debounced function await new Promise((resolve) => setTimeout(resolve, 0)); - if (timeout === undefined) return; + if (!context || !context.timeout) return; } - clearTimeout(timeout); - reject?.("Cancelled"); - reset(); + clearTimeout(context.timeout); + context.timeout = undefined; + context.reject("Cancelled"); }; Object.defineProperty(debounced, "pending", { enumerable: true, get() { - return !!timeout; + return !!context?.timeout; }, }); diff --git a/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts b/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts index 781db696..19254d45 100644 --- a/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts +++ b/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts @@ -28,4 +28,31 @@ describe("useDebounce", () => { await new Promise((resolve) => setTimeout(resolve, 200)); expect(fn).not.toHaveBeenCalled(); }); + + testWithEffect("No race contion with running callback", async () => { + let calledNTimes = 0; + + const slowFunction = async () => { + calledNTimes++; + + await new Promise((resolve) => setTimeout(resolve, 100)); + }; + const debounced = useDebounce(slowFunction, 100); + + expect(calledNTimes).toBe(0); + debounced(); + expect(calledNTimes).toBe(0); + expect(debounced.pending).toBe(true); + + await new Promise((resolve) => setTimeout(resolve, 110)); + expect(calledNTimes).toBe(1); + expect(debounced.pending).toBe(false); + debounced(); + expect(calledNTimes).toBe(1); + expect(debounced.pending).toBe(true); + + await new Promise((resolve) => setTimeout(resolve, 110)); + expect(debounced.pending).toBe(false); + expect(calledNTimes).toBe(2); + }); }); From 7578f21ab6d0486666f3456551496048e9c0c192 Mon Sep 17 00:00:00 2001 From: Rubin Raithel <33808743+Coronon@users.noreply.github.com> Date: Sat, 2 Nov 2024 02:45:04 +0100 Subject: [PATCH 2/3] chore: add changeset --- .changeset/three-owls-matter.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/three-owls-matter.md diff --git a/.changeset/three-owls-matter.md b/.changeset/three-owls-matter.md new file mode 100644 index 00000000..5df6f301 --- /dev/null +++ b/.changeset/three-owls-matter.md @@ -0,0 +1,5 @@ +--- +"runed": patch +--- + +fix: useDebounce race condition (#139) From 6d152167db8f194e90cdf282100b32ddb5aebd01 Mon Sep 17 00:00:00 2001 From: Coronon <33808743+Coronon@users.noreply.github.com> Date: Sat, 2 Nov 2024 02:56:11 +0100 Subject: [PATCH 3/3] fix: don't reuse promise after cancel --- .../useDebounce/useDebounce.svelte.ts | 2 +- .../useDebounce/useDebounce.test.svelte.ts | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts b/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts index 1626bb49..c98cebe7 100644 --- a/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts +++ b/packages/runed/src/lib/utilities/useDebounce/useDebounce.svelte.ts @@ -88,8 +88,8 @@ export function useDebounce( } clearTimeout(context.timeout); - context.timeout = undefined; context.reject("Cancelled"); + context = null; }; Object.defineProperty(debounced, "pending", { diff --git a/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts b/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts index 19254d45..9e7276b2 100644 --- a/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts +++ b/packages/runed/src/lib/utilities/useDebounce/useDebounce.test.svelte.ts @@ -29,6 +29,29 @@ describe("useDebounce", () => { expect(fn).not.toHaveBeenCalled(); }); + testWithEffect("Doesn't reuse promise after cancel", async () => { + // Same as above + const fn = vi.fn(); + const debounced = useDebounce(fn, 100); + + expect(fn).not.toHaveBeenCalled(); + debounced().catch(() => {}); + expect(fn).not.toHaveBeenCalled(); + expect(debounced.pending).toBe(true); + debounced.cancel(); + expect(debounced.pending).toBe(false); + await new Promise((resolve) => setTimeout(resolve, 200)); + expect(fn).not.toHaveBeenCalled(); + + // New test + let wasCatchCalled = false; + debounced().catch(() => (wasCatchCalled = true)); + expect(wasCatchCalled).toBe(false); + await new Promise((resolve) => setTimeout(resolve, 110)); + expect(wasCatchCalled).toBe(false); + expect(fn).toHaveBeenCalledTimes(1); + }); + testWithEffect("No race contion with running callback", async () => { let calledNTimes = 0;