From b93d11dfa16dcd1e312d891266f1c712e6475d94 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Wed, 18 May 2022 13:25:44 +0200 Subject: [PATCH 1/2] fix _o bug --- _internal/utils/mutate.ts | 12 +++++-- mutation/index.ts | 1 + test/use-swr-local-mutation.test.tsx | 51 ++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/_internal/utils/mutate.ts b/_internal/utils/mutate.ts index cc2544956..c8e3903df 100644 --- a/_internal/utils/mutate.ts +++ b/_internal/utils/mutate.ts @@ -126,6 +126,9 @@ export const internalMutate = async ( // If we should write back the cache after request. if (populateCache) { + // Always update or reset the error. + const newState: State = { error } + if (!error) { // Transform the result into data. if (isFunction(populateCache)) { @@ -133,11 +136,10 @@ export const internalMutate = async ( } // Only update cached data if there's no error. Data can be `undefined` here. - set({ data }) + newState.data = data } - // Always update or reset the error and original data field. - set({ error, _o: UNDEFINED }) + set(newState) } // Reset the timestamp to mark the mutation has ended. @@ -146,6 +148,10 @@ export const internalMutate = async ( // Update existing SWR Hooks' internal states: const res = await startRevalidate() + // The mutation and revalidation are ended, we can reset the original data since + // the data is not an optimistic value anymore. + set({ _o: UNDEFINED }) + // Throw error or return data if (error) throw error return populateCache ? res : data diff --git a/mutation/index.ts b/mutation/index.ts index c24f60bf9..0cf541241 100644 --- a/mutation/index.ts +++ b/mutation/index.ts @@ -61,6 +61,7 @@ const mutation = (() => try { const data = await mutate( serializedKey, + // FIXME: Error shouldn't be broadcasted here. (fetcher as any)(resolvedKey, { arg }), options ) diff --git a/test/use-swr-local-mutation.test.tsx b/test/use-swr-local-mutation.test.tsx index 2a11671f6..e690b764e 100644 --- a/test/use-swr-local-mutation.test.tsx +++ b/test/use-swr-local-mutation.test.tsx @@ -1229,6 +1229,57 @@ describe('useSWR - local mutation', () => { expect(previousValue2).toBe(0) }) + it('should rollback to the original value after multiple mutations', async () => { + const key = createKey() + const renderedData = [] + let mutate + let serverData = 'foo' + + function Page() { + const { data, mutate: boundMutate } = useSWR(key, () => + createResponse(serverData, { delay: 20 }) + ) + mutate = boundMutate + if ( + !renderedData.length || + renderedData[renderedData.length - 1] !== data + ) { + renderedData.push(data) + } + return
data: {String(data)}
+ } + + // data == "foo" + renderWithConfig() + await screen.findByText('data: foo') + + // data == "bar" + await executeWithoutBatching(async () => { + await mutate( + createResponse('bar', { delay: 20 }).then(r => (serverData = r)), + { + optimisticData: 'bar', + populateCache: false + } + ) + }) + + try { + // data == "baz", then reverted back to "bar" + await executeWithoutBatching(() => + mutate(createResponse(new Error(), { delay: 20 }), { + optimisticData: 'baz', + revalidate: false + }) + ) + } catch (_) { + // Ignore + } + + await sleep(30) + expect(renderedData).toEqual([undefined, 'foo', 'bar', 'baz', 'bar']) + }) + it('should not rollback optimistic updates if `rollbackOnError`', async () => { const key = createKey() const renderedData = [] From db069b12c89e96bda1463e8399180dfb8f0e1ef0 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Wed, 18 May 2022 19:37:08 +0200 Subject: [PATCH 2/2] fix race conditions --- _internal/utils/mutate.ts | 42 +++++++++++-------- test/use-swr-local-mutation.test.tsx | 63 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 18 deletions(-) diff --git a/_internal/utils/mutate.ts b/_internal/utils/mutate.ts index c8e3903df..578a710fa 100644 --- a/_internal/utils/mutate.ts +++ b/_internal/utils/mutate.ts @@ -42,8 +42,8 @@ export const internalMutate = async ( const [get, set] = createCacheHelper< Data, State & { - // The original data. - _o?: Data + // The previously committed data. + _c?: Data } >(cache, key) const [EVENT_REVALIDATORS, MUTATION, FETCH] = SWRGlobalState.get( @@ -80,21 +80,27 @@ export const internalMutate = async ( const hasOptimisticData = !isUndefined(optimisticData) const state = get() - const currentData = state.data - const originalData = isUndefined(state._o) ? currentData : state._o + + // `displayedData` is the current value on screen. It could be the optimistic value + // that is going to be overridden by a `committedData`, or get reverted back. + // `committedData` is the validated value that comes from a fetch or mutation. + const displayedData = state.data + const committedData = isUndefined(state._c) ? displayedData : state._c // Do optimistic data update. if (hasOptimisticData) { optimisticData = isFunction(optimisticData) - ? optimisticData(originalData) + ? optimisticData(committedData) : optimisticData - set({ data: optimisticData, _o: originalData }) + + // When we set optimistic data, backup the current committedData data in `_c`. + set({ data: optimisticData, _c: committedData }) } if (isFunction(data)) { // `data` is a function, call it passing current cache value. try { - data = (data as MutatorCallback)(originalData) + data = (data as MutatorCallback)(committedData) } catch (err) { // If it throws an error synchronously, we shouldn't update the cache. error = err @@ -119,27 +125,27 @@ export const internalMutate = async ( // Rollback. Always populate the cache in this case but without // transforming the data. populateCache = true - data = originalData - set({ data: originalData }) + data = committedData + + // Reset data to be the latest committed data, and clear the `_c` value. + set({ data, _c: UNDEFINED }) } } // If we should write back the cache after request. if (populateCache) { - // Always update or reset the error. - const newState: State = { error } - if (!error) { // Transform the result into data. if (isFunction(populateCache)) { - data = populateCache(data, originalData) + data = populateCache(data, committedData) } // Only update cached data if there's no error. Data can be `undefined` here. - newState.data = data + set({ data, _c: UNDEFINED }) } - set(newState) + // Always update error and original data here. + set({ error }) } // Reset the timestamp to mark the mutation has ended. @@ -148,9 +154,9 @@ export const internalMutate = async ( // Update existing SWR Hooks' internal states: const res = await startRevalidate() - // The mutation and revalidation are ended, we can reset the original data since - // the data is not an optimistic value anymore. - set({ _o: UNDEFINED }) + // The mutation and revalidation are ended, we can clear it since the data is + // not an optimistic value anymore. + set({ _c: UNDEFINED }) // Throw error or return data if (error) throw error diff --git a/test/use-swr-local-mutation.test.tsx b/test/use-swr-local-mutation.test.tsx index e690b764e..1347f1ff1 100644 --- a/test/use-swr-local-mutation.test.tsx +++ b/test/use-swr-local-mutation.test.tsx @@ -1280,6 +1280,69 @@ describe('useSWR - local mutation', () => { expect(renderedData).toEqual([undefined, 'foo', 'bar', 'baz', 'bar']) }) + it('should rollback to the original value after multiple mutations (2)', async () => { + const key = createKey() + const renderedData = [] + let mutate + let serverData = 'foo' + + function Page() { + const { data, mutate: boundMutate } = useSWR(key, () => + createResponse(serverData, { delay: 20 }) + ) + mutate = boundMutate + if ( + !renderedData.length || + renderedData[renderedData.length - 1] !== data + ) { + renderedData.push(data) + } + return
data: {String(data)}
+ } + + // data == "foo" + renderWithConfig() + await screen.findByText('data: foo') + + // Here m1 and m2 have overlap and m1 will be discarded. + await executeWithoutBatching(async () => { + const m1 = mutate( + createResponse('bar', { delay: 30 }).then(r => (serverData = r)), + { + optimisticData: 'bar', + populateCache: false + } + ) + + await sleep(10) + + const m2 = mutate( + createResponse('baz', { delay: 30 }).then(r => (serverData = r)) + ) + + await m1 + await m2 + }) + + try { + // data == "qux", then reverted back to "baz" + await executeWithoutBatching(() => + mutate(createResponse(new Error(), { delay: 20 }), { + optimisticData: 'qux', + revalidate: false + }) + ) + } catch (_) { + // Ignore + } + + // data: "foo" -> "bar" -> "baz" -> "qux" -> "baz" + // ^ optimistic ^ error + + await sleep(30) + expect(renderedData).toEqual([undefined, 'foo', 'bar', 'baz', 'qux', 'baz']) + }) + it('should not rollback optimistic updates if `rollbackOnError`', async () => { const key = createKey() const renderedData = []