Skip to content

RFC: add "merge" functionality #1059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions packages/toolkit/src/query/core/buildSlice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
isAnyOf,
isFulfilled,
isRejectedWithValue,
createNextState,
} from '@reduxjs/toolkit'
import type {
CombinedState as CombinedQueryState,
Expand All @@ -27,6 +28,7 @@ import { calculateProvidedByThunk } from './buildThunks'
import type {
AssertTagTypes,
EndpointDefinitions,
QueryDefinition,
} from '../endpointDefinitions'
import type { Patch } from 'immer'
import { applyPatches } from 'immer'
Expand Down Expand Up @@ -156,11 +158,37 @@ export function buildSlice({
meta.arg.queryCacheKey,
(substate) => {
if (substate.requestId !== meta.requestId) return
const { merge } = definitions[
meta.arg.endpointName
] as QueryDefinition<any, any, any, any>
substate.status = QueryStatus.fulfilled
substate.data =
definitions[meta.arg.endpointName].structuralSharing ?? true
? copyWithStructuralSharing(substate.data, payload)
: payload

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(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nested createNextState is not causing any problems? I do remember something vaguely that this had caused problems in the past.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Immer had some issues with nested drafts in earlier versions. That behavior has been made more consistent now. I did some testing here with both mutate and return to make sure they work, and also confirmed it throws if you mutate + return.

substate.data,
(draftSubstateData) => {
// As usual with Immer, you can mutate _or_ return inside here, but not both
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary return if you omit the method body. I don't know if minifiers would eliminate that, but why take the chance they don't? :)

return merge(draftSubstateData, payload)
}
)
substate.data = newData
} else {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this case could be skipped if you test for substate.data up there together with the if (merge) { - the fallthrough to copyWithStructuralSharing would be totally fine

// 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
}
Expand Down
19 changes: 19 additions & 0 deletions packages/toolkit/src/query/endpointDefinitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,25 @@ export interface QueryExtraOptions<
invalidatesTags?: never

serializeQueryArgs?: SerializeQueryArgs<any>

/**
* 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 for mentioning this. I hope people read it.

* 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?(
currentCacheData: ResultType,
responseData: ResultType
): ResultType | void
}

export type QueryDefinition<
Expand Down
101 changes: 101 additions & 0 deletions packages/toolkit/src/query/tests/buildSlice.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ const authSlice = createSlice({

const storeRef = setupApiStore(api, { auth: authSlice.reducer })

function delay(ms: number) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see waitForFakeTimer from helpers.ts

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()
Expand Down Expand Up @@ -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<Todo[], void>({
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<Todo[], void>({
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' }])
})
})