From 9462d551e71d1b6919beb47f040c025f87ebf941 Mon Sep 17 00:00:00 2001 From: David Maskasky Date: Wed, 13 Nov 2024 20:48:30 -0800 Subject: [PATCH 1/2] add failing tests for 'should mount and trigger listeners even when an error is thrown' --- tests/vanilla/store.test.tsx | 212 +++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) diff --git a/tests/vanilla/store.test.tsx b/tests/vanilla/store.test.tsx index 859af8730c..68f941cc76 100644 --- a/tests/vanilla/store.test.tsx +++ b/tests/vanilla/store.test.tsx @@ -646,3 +646,215 @@ describe('should invoke flushPending only after all atoms are updated (#2804)', ]) }) }) + +describe('should mount and trigger listeners even when an error is thrown', () => { + it('in asynchronous read', async () => { + const store = createStore() + const a = atom(0) + a.onMount = vi.fn() + const e = atom( + () => { + throw new Error('error') + }, + () => {}, + ) + e.onMount = vi.fn() + const b = atom((get) => { + setTimeout(() => { + get(a) + try { + get(e) + } catch {} + }) + }) + store.sub(b, () => {}) + await new Promise((r) => setTimeout(r)) + expect(a.onMount).toHaveBeenCalledOnce() + expect(e.onMount).toHaveBeenCalledOnce() + }) + + it('in read setSelf', async () => { + const store = createStore() + const a = atom(0) + const e = atom( + () => { + throw new Error('error') + }, + () => {}, + ) + const b = atom( + (_, { setSelf }) => { + setTimeout(() => { + try { + setSelf() + } catch {} + }) + }, + (get, set) => { + set(a, 1) + get(e) + }, + ) + const listener = vi.fn() + store.sub(a, listener) + store.sub(b, () => {}) + await new Promise((r) => setTimeout(r)) + expect(listener).toHaveBeenCalledOnce() + }) + + it('in read promise on settled', async () => { + const store = createStore() + const a = atom(0) + a.onMount = vi.fn() + const e = atom( + () => { + throw new Error('error') + }, + () => {}, + ) + const b = atom(async (get) => { + await new Promise((r) => setTimeout(r)) + get(a) + get(e) + }) + store.sub(b, () => {}) + await new Promise((r) => setTimeout(r)) + expect(a.onMount).toHaveBeenCalledOnce() + }) + + it('in asynchronous write', async () => { + const store = createStore() + const a = atom(0) + const e = atom(() => { + throw new Error('error') + }) + const b = atom(null, (get, set) => { + set(a, 1) + get(e) + }) + const w = atom(null, async (get, set) => { + setTimeout(() => { + try { + set(b) + } catch {} + }) + }) + const listener = vi.fn() + store.sub(a, listener) + store.set(w) + await new Promise((r) => setTimeout(r)) + expect(listener).toHaveBeenCalledOnce() + }) + + it('in synchronous write', () => { + const store = createStore() + const a = atom(0) + a.debugLabel = 'a' + const e = atom(() => { + throw new Error('error') + }) + e.debugLabel = 'e' + const b = atom(null, (get, set) => { + set(a, 1) + get(e) + }) + b.debugLabel = 'b' + const listener = vi.fn() + store.sub(a, listener) + try { + store.set(b) + } catch {} + expect(listener).toHaveBeenCalledOnce() + }) + + it('in onmount/onunmount asynchronous setAtom', async () => { + const store = createStore() + const a = atom(0) + const e = atom(() => { + throw new Error('error') + }) + const b = atom(null, (get, set) => { + set(a, (v) => ++v) + get(e) + }) + b.onMount = (setAtom) => { + setTimeout(() => { + try { + setAtom() + } catch {} + }) + return () => { + setTimeout(() => { + try { + setAtom() + } catch {} + }) + } + } + const listener = vi.fn() + store.sub(a, listener) + const unsub = store.sub(b, () => {}) + await new Promise((r) => setTimeout(r)) + expect(listener).toHaveBeenCalledOnce() + listener.mockClear() + unsub() + await new Promise((r) => setTimeout(r)) + expect(listener).toHaveBeenCalledOnce() + }) + + it('in synchronous onmount', () => { + const store = createStore() + const a = atom(0) + const aUnmount = vi.fn() + a.onMount = vi.fn(() => aUnmount) + const b = atom( + (get) => get(a), + () => {}, + ) + b.onMount = () => { + throw new Error('error') + } + try { + store.sub(b, () => {}) + } catch {} + expect(a.onMount).toHaveBeenCalledOnce() + }) + + it('in synchronous onunmount', () => { + const store = createStore() + const a = atom(0) + const aUnmount = vi.fn() + a.onMount = () => aUnmount + const b = atom( + (get) => get(a), + () => {}, + ) + b.onMount = () => () => { + throw new Error('error') + } + const unsub = store.sub(b, () => {}) + try { + unsub() + } catch {} + expect(aUnmount).toHaveBeenCalledOnce() + }) + + it('in synchronous listener', () => { + const store = createStore() + const a = atom(0) + const e = atom(0) + const b = atom(null, (_, set) => { + set(a, 1) + set(e, 1) + }) + store.sub(e, () => { + throw new Error('error') + }) + const listener = vi.fn() + store.sub(a, listener) + try { + store.set(b) + } catch {} + expect(listener).toHaveBeenCalledOnce() + }) +}) From 004e873e00488c9c67578b250b638d05aa02546e Mon Sep 17 00:00:00 2001 From: David Maskasky Date: Wed, 13 Nov 2024 21:08:53 -0800 Subject: [PATCH 2/2] improve error handling for flushPending --- src/vanilla/store.ts | 17 +++++++++++++++-- tests/vanilla/store.test.tsx | 36 +++++++++++++++++++++++++++--------- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/vanilla/store.ts b/src/vanilla/store.ts index 190d0766de..3684c00e65 100644 --- a/src/vanilla/store.ts +++ b/src/vanilla/store.ts @@ -199,14 +199,27 @@ const addPendingFunction = (pending: Pending, fn: () => void) => { } const flushPending = (pending: Pending) => { + let error: unknown | undefined + const call = (fn: () => void) => { + try { + fn() + } catch (e) { + if (!error) { + error = e + } + } + } while (pending[1].size || pending[2].size) { pending[0].clear() const atomStates = new Set(pending[1].values()) pending[1].clear() const functions = new Set(pending[2]) pending[2].clear() - atomStates.forEach((atomState) => atomState.m?.l.forEach((l) => l())) - functions.forEach((fn) => fn()) + atomStates.forEach((atomState) => atomState.m?.l.forEach(call)) + functions.forEach(call) + } + if (error) { + throw error } } diff --git a/tests/vanilla/store.test.tsx b/tests/vanilla/store.test.tsx index 68f941cc76..8a765240a0 100644 --- a/tests/vanilla/store.test.tsx +++ b/tests/vanilla/store.test.tsx @@ -664,7 +664,9 @@ describe('should mount and trigger listeners even when an error is thrown', () = get(a) try { get(e) - } catch {} + } catch { + // expect error + } }) }) store.sub(b, () => {}) @@ -687,7 +689,9 @@ describe('should mount and trigger listeners even when an error is thrown', () = setTimeout(() => { try { setSelf() - } catch {} + } catch { + // expect error + } }) }, (get, set) => { @@ -736,7 +740,9 @@ describe('should mount and trigger listeners even when an error is thrown', () = setTimeout(() => { try { set(b) - } catch {} + } catch { + // expect error + } }) }) const listener = vi.fn() @@ -763,7 +769,9 @@ describe('should mount and trigger listeners even when an error is thrown', () = store.sub(a, listener) try { store.set(b) - } catch {} + } catch { + // expect error + } expect(listener).toHaveBeenCalledOnce() }) @@ -781,13 +789,17 @@ describe('should mount and trigger listeners even when an error is thrown', () = setTimeout(() => { try { setAtom() - } catch {} + } catch { + // expect error + } }) return () => { setTimeout(() => { try { setAtom() - } catch {} + } catch { + // expect error + } }) } } @@ -816,7 +828,9 @@ describe('should mount and trigger listeners even when an error is thrown', () = } try { store.sub(b, () => {}) - } catch {} + } catch { + // expect error + } expect(a.onMount).toHaveBeenCalledOnce() }) @@ -835,7 +849,9 @@ describe('should mount and trigger listeners even when an error is thrown', () = const unsub = store.sub(b, () => {}) try { unsub() - } catch {} + } catch { + // expect error + } expect(aUnmount).toHaveBeenCalledOnce() }) @@ -854,7 +870,9 @@ describe('should mount and trigger listeners even when an error is thrown', () = store.sub(a, listener) try { store.set(b) - } catch {} + } catch { + // expect error + } expect(listener).toHaveBeenCalledOnce() }) })