RFC: add "merge" functionality

[phryneas]

This might be useful in "streaming data" scenarios where a .refresh call should not remove old data, but rather just merge current values into the existing dataset, e.g. as a "re-sync".

Not 100% sure about the usefulness though, hence the "RFC" in the title.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fdb4ab6:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration
@examples-query-react/basic	Configuration
@examples-query-react/advanced	Configuration
@examples-action-listener/counter	Configuration

[netlify]

✅ Deploy Preview for redux-starter-kit-docs ready!

Name	Link
🔨 Latest commit	fdb4ab6
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/62f83cff9cac340008658ffa
😎 Deploy Preview	https://deploy-preview-1059--redux-starter-kit-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[aldreth]

@phryneas This is what I came to github to ask about! I currently use a thunk to merge new data into state & can't see a way of replicating that with RTK Query.

[phryneas]

@aldreth Currently I'm at the point of "almost all usecases I can think of go against the idea of RTKQ itself and would probably be more dangerous that useful for almost everyone". But with enough good use cases I might change my mind. Can you elaborate a bit on your usecase?

[aldreth]

@phryneas I am basically showing a list of events. The API, which I don't control, doesn't return today's events. I save the redux state into local storage, so if a user has previously visited the site I can merge new events into state and still show events for today.
I can see that it's an edge case - the right thing to do would be to update the API, but it's an local government thing, so I have no control over it.

[phryneas]

In that case I'd recommed to use an extraReducer in a slice you have control over to save that data to the side. You should not persist an RTKQ cache slice.

[ryota-murakami]

Is this will become default strategy of refetch()?
Actually common use case doe't affect this change i think.

[phryneas]

@ryota-murakami no, this will not be the default - the default will still be replacing the last result.
This will be an edge case feature that should be used with extreme care - for example in an endless scrolling situation it could easily lead to memory leaks if the user does not clean up old entries. The cache entry would just grow and grow.

Even for endless scrolling, it would still be advisable for almost all situations to use multiple useQuery hook calls, as that way RTK-Q can do the cleanup.

[ryota-murakami]

@phryneas Thanks, that's make sense!

[markerikson]

Here's an example of where this might be useful, from Replay's codebase, along with the possibility of customizing cache key generation.

Replay stores info on "source actors". I'm still kinda hazy on the difference between a "source" and a "source actor", but we have a reducer that stores an entry per "source actor" along with its async request status, and that entry has a few different fields, including details on "breakable lines" and "hit counts per line":

https://github.com/replayio/devtools/blob/61525ea3f097926435d6d03f94a7c9895cca71a8/src/devtools/client/debugger/src/reducers/source-actors.ts#L30-L44

We fetch the basic data up front, and then fill in more of that detail later based on user interactions (hovering over a line number fetches hit counts, etc).

The current fetching logic is really ugly and uses an internal abstraction called a "memoizableAction":

https://github.com/replayio/devtools/blob/61525ea3f097926435d6d03f94a7c9895cca71a8/src/devtools/client/debugger/src/actions/source-actors.ts#L85-L119

and the corresponding reducer logic then has to figure out:

• Do we have an entry for this ID?
• what's its status?
• okay, now use an "update resource" util to find that item and update these couple fields, including merging the hit counts data into an existing array:

https://github.com/replayio/devtools/blob/61525ea3f097926435d6d03f94a7c9895cca71a8/src/devtools/client/debugger/src/reducers/source-actors.ts#L195-L228

function updateBreakpointHitCounts(
  state: SourceActorsState,
  action: SetSourceActorBreakpointHitCountsAction
) {
  // @ts-ignore start/done mismatch
  const value = asyncActionAsValue<SetSourceActorBreakpointHitCountsAction["value"]>(action);
  if (value.state === "pending") {
    return state;
  }

  const { id: sourceId } = action;

  if (!hasResource(state, sourceId)) {
    return state;
  }

  const {
    breakpointHitCountLinesKnown: currentLinesKnown,
    breakpointHitCounts: currentBreakpointHitCounts,
  } = {
    ...state.values[sourceId],
  };

  return updateResources(state, [
    {
      breakpointHitCountLinesKnown: mergeRanges([
        ...(currentLinesKnown || []),
        { max: action.value.max, min: action.value.min },
      ]),
      breakpointHitCounts: [...(currentBreakpointHitCounts || []), ...action.value.hits],
      id: sourceId,
    },
  ]);
}

This all feels like code that could potentially be replaced by RTK Query endpoint handling... except that we don't have a way to merge in more data over time.

And, imagine that I do something like getSourceLineHits({sourceId, line: 123}), and then again with line: 456. I'd like them both to resolve to the same sourceId entry.

So that's where I imagine that a customizeCacheKey option would let me drop the line field from the cache key so that we're really just keying off sourceId, but still run the request, and then use merge to update the existing entry.

[markerikson]

I've rebased this PR and would like to finalize it. But, I don't think the behavior in this initial implementation is what we'd want.

Right now it's:

              const { merge = (x: any) => x } = definitions[
                meta.arg.endpointName
              ] as QueryDefinition<any, any, any, any>
              substate.status = QueryStatus.fulfilled
              let newData = merge(payload, substate.data)

              substate.data =
                definitions[meta.arg.endpointName].structuralSharing ?? true
                  ? copyWithStructuralSharing(substate.data, newData)
                  : newData

If you're supplying a merge option, that means you're now trying to mutate substate.data. which means that at that point newData and structuralSharing are a moot point, right?

I feel like if you've supplied a merge callback, it's now your responsibility to do the update work, and we should make that mutually exclusive with the replacement or structural sharing behavior.

[markerikson]

I successfully updated the logic to look like this:

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
}

and added a couple tests.

This looks pretty good to me.

[phryneas]

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

[markerikson]

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.

[phryneas]

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? :)

[phryneas]

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

[phryneas]

💯 for mentioning this. I hope people read it.

[phryneas]

see waitForFakeTimer from helpers.ts