From 4860a8909315a7ca311583b4a0691c8756d9749c Mon Sep 17 00:00:00 2001 From: Lenz Weber Date: Sun, 16 May 2021 11:04:32 +0200 Subject: [PATCH 1/2] RFC: add "merge" functionality --- packages/toolkit/src/query/core/buildSlice.ts | 10 ++++++++-- packages/toolkit/src/query/endpointDefinitions.ts | 12 ++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/toolkit/src/query/core/buildSlice.ts b/packages/toolkit/src/query/core/buildSlice.ts index 8e1c9e05d1..d5c340b093 100644 --- a/packages/toolkit/src/query/core/buildSlice.ts +++ b/packages/toolkit/src/query/core/buildSlice.ts @@ -27,6 +27,7 @@ import { calculateProvidedByThunk } from './buildThunks' import type { AssertTagTypes, EndpointDefinitions, + QueryDefinition, } from '../endpointDefinitions' import type { Patch } from 'immer' import { applyPatches } from 'immer' @@ -156,11 +157,16 @@ export function buildSlice({ meta.arg.queryCacheKey, (substate) => { if (substate.requestId !== meta.requestId) return + const { merge = (x: any) => x } = definitions[ + meta.arg.endpointName + ] as QueryDefinition substate.status = QueryStatus.fulfilled + let newData = merge(payload, substate.data) + substate.data = definitions[meta.arg.endpointName].structuralSharing ?? true - ? copyWithStructuralSharing(substate.data, payload) - : payload + ? copyWithStructuralSharing(substate.data, newData) + : newData delete substate.error substate.fulfilledTimeStamp = meta.fulfilledTimeStamp } diff --git a/packages/toolkit/src/query/endpointDefinitions.ts b/packages/toolkit/src/query/endpointDefinitions.ts index 02293cc41c..6c5b419611 100644 --- a/packages/toolkit/src/query/endpointDefinitions.ts +++ b/packages/toolkit/src/query/endpointDefinitions.ts @@ -275,6 +275,18 @@ export interface QueryExtraOptions< invalidatesTags?: never serializeQueryArgs?: SerializeQueryArgs + + /** + * Can be provided to merge the current cache value into the new cache value. + * + * Useful if you don't want a new request to completely override the current cache value, + * maybe because you have manually updated it from another source and don't want those + * updates to get lost. + */ + merge?( + newCacheValue: ResultType, + currentCacheValue: ResultType | undefined + ): ResultType } export type QueryDefinition< From fdb4ab6a741f651e862e4f826d64753ff4dcd133 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sat, 13 Aug 2022 20:08:14 -0400 Subject: [PATCH 2/2] Update `merge` logic to make it optional and handle different use cases - Make `merge` an optional callback - Flip args order to put `currentCacheData` first - Use Immer to handle the "mutate or return" behavior - Only do structural sharing if there's no `merge` --- packages/toolkit/src/query/core/buildSlice.ts | 34 ++++-- .../toolkit/src/query/endpointDefinitions.ts | 13 ++- .../src/query/tests/buildSlice.test.ts | 101 ++++++++++++++++++ 3 files changed, 139 insertions(+), 9 deletions(-) diff --git a/packages/toolkit/src/query/core/buildSlice.ts b/packages/toolkit/src/query/core/buildSlice.ts index d5c340b093..07ba888188 100644 --- a/packages/toolkit/src/query/core/buildSlice.ts +++ b/packages/toolkit/src/query/core/buildSlice.ts @@ -6,6 +6,7 @@ import { isAnyOf, isFulfilled, isRejectedWithValue, + createNextState, } from '@reduxjs/toolkit' import type { CombinedState as CombinedQueryState, @@ -157,16 +158,37 @@ export function buildSlice({ meta.arg.queryCacheKey, (substate) => { if (substate.requestId !== meta.requestId) return - const { merge = (x: any) => x } = definitions[ + const { merge } = definitions[ meta.arg.endpointName ] as QueryDefinition substate.status = QueryStatus.fulfilled - let newData = merge(payload, substate.data) - substate.data = - definitions[meta.arg.endpointName].structuralSharing ?? true - ? copyWithStructuralSharing(substate.data, newData) - : newData + if (merge) { + if (substate.data !== undefined) { + // There's existing cache data. Let the user merge it in themselves. + // We're already inside an Immer-powered reducer, and the user could just mutate `substate.data` + // themselves inside of `merge()`. But, they might also want to return a new value. + // Try to let Immer figure that part out, save the result, and assign it to `substate.data`. + let newData = createNextState( + substate.data, + (draftSubstateData) => { + // As usual with Immer, you can mutate _or_ return inside here, but not both + return merge(draftSubstateData, payload) + } + ) + substate.data = newData + } else { + // Presumably a fresh request. Just cache the response data. + substate.data = payload + } + } else { + // Assign or safely update the cache data. + substate.data = + definitions[meta.arg.endpointName].structuralSharing ?? true + ? copyWithStructuralSharing(substate.data, payload) + : payload + } + delete substate.error substate.fulfilledTimeStamp = meta.fulfilledTimeStamp } diff --git a/packages/toolkit/src/query/endpointDefinitions.ts b/packages/toolkit/src/query/endpointDefinitions.ts index 6c5b419611..e8f2a1f626 100644 --- a/packages/toolkit/src/query/endpointDefinitions.ts +++ b/packages/toolkit/src/query/endpointDefinitions.ts @@ -278,15 +278,22 @@ export interface QueryExtraOptions< /** * Can be provided to merge the current cache value into the new cache value. + * If supplied, no automatic structural sharing will be applied - it's up to + * you to update the cache appropriately. + * + * Since this is wrapped with Immer, you , you may either mutate the `currentCacheValue` directly, + * or return a new value, but _not_ both at once. * + * + * Will only be called if the existing `currentCacheValue` is not `undefined`. * * Useful if you don't want a new request to completely override the current cache value, * maybe because you have manually updated it from another source and don't want those * updates to get lost. */ merge?( - newCacheValue: ResultType, - currentCacheValue: ResultType | undefined - ): ResultType + currentCacheData: ResultType, + responseData: ResultType + ): ResultType | void } export type QueryDefinition< diff --git a/packages/toolkit/src/query/tests/buildSlice.test.ts b/packages/toolkit/src/query/tests/buildSlice.test.ts index 6be8ccc6fb..1918863739 100644 --- a/packages/toolkit/src/query/tests/buildSlice.test.ts +++ b/packages/toolkit/src/query/tests/buildSlice.test.ts @@ -29,6 +29,10 @@ const authSlice = createSlice({ const storeRef = setupApiStore(api, { auth: authSlice.reducer }) +function delay(ms: number) { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + it('only resets the api state when resetApiState is dispatched', async () => { storeRef.store.dispatch({ type: 'unrelated' }) // trigger "registered middleware" into place const initialState = storeRef.store.getState() @@ -77,3 +81,100 @@ it('only resets the api state when resetApiState is dispatched', async () => { expect(storeRef.store.getState()).toEqual(initialState) }) + +describe.only('`merge` callback', () => { + const baseQuery = (args?: any) => ({ data: args }) + + interface Todo { + id: string + text: string + } + + it('Calls `merge` once there is existing data, and allows mutations of cache state', async () => { + let mergeCalled = false + let queryFnCalls = 0 + const todoTexts = ['A', 'B', 'C', 'D'] + + const api = createApi({ + baseQuery, + endpoints: (build) => ({ + getTodos: build.query({ + async queryFn() { + const text = todoTexts[queryFnCalls] + return { data: [{ id: `${queryFnCalls++}`, text }] } + }, + merge(currentCacheValue, responseData) { + mergeCalled = true + currentCacheValue.push(...responseData) + }, + }), + }), + }) + + const storeRef = setupApiStore(api, undefined, { + withoutTestLifecycles: true, + }) + + const selectTodoEntry = api.endpoints.getTodos.select() + + const res = storeRef.store.dispatch(api.endpoints.getTodos.initiate()) + await res + expect(mergeCalled).toBe(false) + const todoEntry1 = selectTodoEntry(storeRef.store.getState()) + expect(todoEntry1.data).toEqual([{ id: '0', text: 'A' }]) + + res.refetch() + + await delay(10) + + expect(mergeCalled).toBe(true) + const todoEntry2 = selectTodoEntry(storeRef.store.getState()) + + expect(todoEntry2.data).toEqual([ + { id: '0', text: 'A' }, + { id: '1', text: 'B' }, + ]) + }) + + it('Allows returning a different value from `merge`', async () => { + let firstQueryFnCall = true + + const api = createApi({ + baseQuery, + endpoints: (build) => ({ + getTodos: build.query({ + async queryFn() { + const item = firstQueryFnCall + ? { id: '0', text: 'A' } + : { id: '1', text: 'B' } + firstQueryFnCall = false + return { data: [item] } + }, + merge(currentCacheValue, responseData) { + return responseData + }, + }), + }), + }) + + const storeRef = setupApiStore(api, undefined, { + withoutTestLifecycles: true, + }) + + const selectTodoEntry = api.endpoints.getTodos.select() + + const res = storeRef.store.dispatch(api.endpoints.getTodos.initiate()) + await res + + const todoEntry1 = selectTodoEntry(storeRef.store.getState()) + expect(todoEntry1.data).toEqual([{ id: '0', text: 'A' }]) + + res.refetch() + + await delay(10) + + const todoEntry2 = selectTodoEntry(storeRef.store.getState()) + + expect(todoEntry2.data).toEqual([{ id: '1', text: 'B' }]) + }) +})