From 60b6493d12c19f39fcf2a4cb5a0a60e5181c7928 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sun, 15 May 2022 12:47:35 +0200 Subject: [PATCH 1/3] fix optimistic ui race conditions --- _internal/utils/mutate.ts | 33 ++++++++++------ package.json | 2 +- test/use-swr-local-mutation.test.tsx | 56 ++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 12 deletions(-) diff --git a/_internal/utils/mutate.ts b/_internal/utils/mutate.ts index 74cbca98b..7035bd6e9 100644 --- a/_internal/utils/mutate.ts +++ b/_internal/utils/mutate.ts @@ -1,5 +1,5 @@ import { serialize } from './serialize' -import { createCacheHelper, isFunction, isUndefined } from './helper' +import { createCacheHelper, isFunction, isUndefined, UNDEFINED } from './helper' import { SWRGlobalState } from './global-state' import { getTimestamp } from './timestamp' import * as revalidateEvents from '../constants' @@ -8,7 +8,8 @@ import { Cache, MutatorCallback, MutatorOptions, - GlobalState + GlobalState, + State } from '../types' export const internalMutate = async ( @@ -38,12 +39,19 @@ export const internalMutate = async ( const [key] = serialize(_key) if (!key) return - const [get, set] = createCacheHelper(cache, key) + const [get, set] = createCacheHelper< + Data, + State & { + // The original data. + _o?: Data + } + >(cache, key) const [EVENT_REVALIDATORS, MUTATION, FETCH] = SWRGlobalState.get( cache ) as GlobalState + const revalidators = EVENT_REVALIDATORS[key] - const startRevalidate = async () => { + const startRevalidate = () => { if (revalidate) { // Invalidate the key by deleting the concurrent request markers so new // requests will not be deduped. @@ -56,6 +64,7 @@ export const internalMutate = async ( } return get().data } + // If there is no new data provided, revalidate the key with current state. if (args.length < 3) { // Revalidate and broadcast state. @@ -70,20 +79,22 @@ export const internalMutate = async ( MUTATION[key] = [beforeMutationTs, 0] const hasOptimisticData = !isUndefined(optimisticData) - const originalData = get().data + const state = get() + const currentData = state.data + const originalData = isUndefined(state._o) ? currentData : state._o // Do optimistic data update. if (hasOptimisticData) { optimisticData = isFunction(optimisticData) - ? optimisticData(originalData) + ? optimisticData(currentData) : optimisticData - set({ data: optimisticData }) + set({ data: optimisticData, _o: originalData }) } if (isFunction(data)) { // `data` is a function, call it passing current cache value. try { - data = (data as MutatorCallback)(originalData) + data = (data as MutatorCallback)(currentData) } catch (err) { // If it throws an error synchronously, we shouldn't update the cache. error = err @@ -118,15 +129,15 @@ export const internalMutate = async ( if (!error) { // Transform the result into data. if (isFunction(populateCache)) { - data = populateCache(data, originalData) + data = populateCache(data, currentData) } // Only update cached data if there's no error. Data can be `undefined` here. set({ data }) } - // Always update or reset the error. - set({ error }) + // Always update or reset the error and original data field. + set({ error, _o: UNDEFINED }) } // Reset the timestamp to mark the mutation has ended. diff --git a/package.json b/package.json index 765d93734..cfd0fd8fd 100644 --- a/package.json +++ b/package.json @@ -82,7 +82,7 @@ "lint": "eslint . --ext .ts,.tsx --cache", "lint:fix": "yarn lint --fix", "coverage": "jest --coverage", - "test": "jest && tsc --noEmit -p test" + "test": "tsc --noEmit -p test && jest" }, "husky": { "hooks": { diff --git a/test/use-swr-local-mutation.test.tsx b/test/use-swr-local-mutation.test.tsx index c81776717..64c2257a5 100644 --- a/test/use-swr-local-mutation.test.tsx +++ b/test/use-swr-local-mutation.test.tsx @@ -1142,6 +1142,62 @@ describe('useSWR - local mutation', () => { expect(renderedData).toEqual([undefined, 0, 'bar', 0, 1]) }) + it('should not revert to optimistic data when rolling back', async () => { + const key = createKey() + const renderedData = [] + let mutate + let previousValue + let previousValue2 + + function Page() { + const { data, mutate: boundMutate } = useSWR(key, () => + createResponse(0, { delay: 20 }) + ) + mutate = boundMutate + + if ( + !renderedData.length || + renderedData[renderedData.length - 1] !== data + ) { + renderedData.push(data) + } + + return
data: {String(data)}
+ } + + renderWithConfig() + await screen.findByText('data: 0') + + await executeWithoutBatching(async () => { + const p1 = mutate(createResponse(new Error(), { delay: 20 }), { + optimisticData: 1 + }) + await sleep(10) + const p2 = mutate( + v => { + previousValue = v + return createResponse(new Error(), { delay: 20 }) + }, + { + optimisticData: v => { + previousValue2 = v + return 2 + } + } + ) + return Promise.all([p1, p2]) + }) + + await sleep(30) + + // It should revert to `0` instead of `1` at the end. + expect(renderedData).toEqual([undefined, 0, 1, 2, 0]) + + // It should receive the latest displayed data instead of the original data. + expect(previousValue).toBe(1) + expect(previousValue2).toBe(1) + }) + it('should not rollback optimistic updates if `rollbackOnError`', async () => { const key = createKey() const renderedData = [] From a69717190f56c79cc1c6270a44b39e2eae4307a1 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sun, 15 May 2022 12:49:44 +0200 Subject: [PATCH 2/3] fix test --- test/use-swr-local-mutation.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/use-swr-local-mutation.test.tsx b/test/use-swr-local-mutation.test.tsx index 64c2257a5..16e9d59ae 100644 --- a/test/use-swr-local-mutation.test.tsx +++ b/test/use-swr-local-mutation.test.tsx @@ -1186,7 +1186,7 @@ describe('useSWR - local mutation', () => { } ) return Promise.all([p1, p2]) - }) + }).catch(_e => {}) await sleep(30) From f580f0b783b1e0c7ce4a31cbdfbdcbebdfd07b00 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sun, 15 May 2022 12:57:59 +0200 Subject: [PATCH 3/3] fix --- _internal/utils/mutate.ts | 6 +++--- test/use-swr-local-mutation.test.tsx | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/_internal/utils/mutate.ts b/_internal/utils/mutate.ts index 7035bd6e9..cc2544956 100644 --- a/_internal/utils/mutate.ts +++ b/_internal/utils/mutate.ts @@ -86,7 +86,7 @@ export const internalMutate = async ( // Do optimistic data update. if (hasOptimisticData) { optimisticData = isFunction(optimisticData) - ? optimisticData(currentData) + ? optimisticData(originalData) : optimisticData set({ data: optimisticData, _o: originalData }) } @@ -94,7 +94,7 @@ export const internalMutate = async ( if (isFunction(data)) { // `data` is a function, call it passing current cache value. try { - data = (data as MutatorCallback)(currentData) + data = (data as MutatorCallback)(originalData) } catch (err) { // If it throws an error synchronously, we shouldn't update the cache. error = err @@ -129,7 +129,7 @@ export const internalMutate = async ( if (!error) { // Transform the result into data. if (isFunction(populateCache)) { - data = populateCache(data, currentData) + data = populateCache(data, originalData) } // Only update cached data if there's no error. Data can be `undefined` here. diff --git a/test/use-swr-local-mutation.test.tsx b/test/use-swr-local-mutation.test.tsx index 16e9d59ae..c149081f8 100644 --- a/test/use-swr-local-mutation.test.tsx +++ b/test/use-swr-local-mutation.test.tsx @@ -1193,9 +1193,9 @@ describe('useSWR - local mutation', () => { // It should revert to `0` instead of `1` at the end. expect(renderedData).toEqual([undefined, 0, 1, 2, 0]) - // It should receive the latest displayed data instead of the original data. - expect(previousValue).toBe(1) - expect(previousValue2).toBe(1) + // It should receive the original displayed data instead of current displayed data. + expect(previousValue).toBe(0) + expect(previousValue2).toBe(0) }) it('should not rollback optimistic updates if `rollbackOnError`', async () => {