implemented upsertQueryData functionality per #1720 #2007

[barnabasJ]

I used the comments in PR #2007 to implement a proof of concept for the upsertQueryData functionality.

I copied and adapted the optimisticUpdate test code to test this.

I added some comments to the does update non existing values test.

• I'm not sure about the behaviour if there are currently no active subscribers.
• The undo might not work as expected.

Please let me know what you think of this

[codesandbox-ci[bot]]

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 06ee327e7f777be2320b77bffdd2e86614238420:

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[bot]]

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

Name	Link
🔨 Latest commit	12c48afb7c8025653191d744cf834470ba9a87fc
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/62601df141bae5000879b664
😎 Deploy Preview	https://deploy-preview-2266--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.

[phryneas]

That's looking pretty good right now!

One scenario I'm concerned about: What happens/should happen when upsertQueryData is called for a cache entry that is currently "in flight"?

From the back of my head: With this current solution, the request in flight would continue and upsertQueryData would not have any effect (due to the condition of initiate).

We could change condition and in that case, the value from upsertQueryData would make it into the cache, but the result from the query would probably never make it in the state.

Now the question: I think the second behaviour feels like the intended behaviour - after all, the upsert call was made after, which indicates that we consider that that is the more "up to date" data. @barnabasJ do you agree? @markerikson do you have thoughts on that?

[phryneas]

(also I'm not sure on the emptyCache name - maybe more along the lines of cacheEntryFound?)

[barnabasJ]

That's looking pretty good right now!

One scenario I'm concerned about: What happens/should happen when upsertQueryData is called for a cache entry that is currently "in flight"?

From the back of my head: With this current solution, the request in flight would continue and upsertQueryData would not have any effect (due to the condition of initiate).

We could change condition and in that case, the value from upsertQueryData would make it into the cache, but the result from the query would probably never make it in the state.

Now the question: I think the second behaviour feels like the intended behaviour - after all, the upsert call was made after, which indicates that we consider that that is the more "up to date" data. @barnabasJ do you agree? @markerikson do you have thoughts on that?

What happens with the patches from updateQueryData if there is a request in flight? They are overwritten, right? If so I think this should work the same way. My gut feeling is the server should be the source of truth.

[phryneas]

Problem is that with the current underlying logic, a reducer will only ever react to the last started request for a cache key and ignore all requests that would possibly run otherwise.

https://github.com/reduxjs/redux-toolkit/blob/8c23cd33971ae5a62bc19764adca6f6be2ed98ec/packages/toolkit/src/query/core/buildSlice.ts#L158

So either we have this start a "pending" and then the according "fulfilled" will be accept (but the one currently running will be ignored), or we don't start this and it will not make it into the cache.

Or we revamp all that logic, of course.

[markerikson]

Huh. I'm not sure this PR is actually running any tests properly.

I just did a rebase against v1.9-integration, and removed the "add/revert" commits in the middle.

Looks like it's trying to run actual tests now.

Also, I don't think the tests got updated to match the "whole values, not updateRecipe callbacks" behavior. working on that now.

[markerikson]

Ok, think I've got the tests updated to match what the current implementation logic is actually doing.

Need to think about the "request in flight case" still, but at least the tests should be accurate.

[markerikson]

So right now it looks like we're actually hitting this check inside of queryThunk if you attempt to call upsertQueryData while a request is in flight:

      // Don't retry a request that's currently in-flight
      if (requestState?.status === 'pending') return false

Because of that, the queryThunk for upsertQueryData bails out entirely, right away.

I've modified the logic so that the if (isForced()) return true check runs first instead. Given that, the upsertQueryData call at least runs.

I've also tried changing the reducer logic to start with:

        .addCase(queryThunk.fulfilled, (draft, { meta, payload }) => {
          updateQuerySubstateIfExists(
            draft,
            meta.arg.queryCacheKey,
            (substate) => {
              if (substate.requestId !== meta.requestId) {
                if (
                  substate.fulfilledTimeStamp &&
                  meta.fulfilledTimeStamp < substate.fulfilledTimeStamp
                ) {
                  return
                }
              }

So, we should end up going with whatever request resolves last.

I don't think this is necessarily "right" or "best" behavior, but I at least know what it does now.

Having said that, I'm still not sure what the final behavior sure be here and would appreciate suggestions.

[markerikson]

Okay. This runs, tests pass, it compiles clean.

Let's merge this in and put out 1.9.0-alpha.1 so people can try it out, and we can tweak the edge case behavior if needed.

[blaine-foreflight]

Hey! So glad I found this! Exactly what I needed. Just wanted to see if there was any documentation coming out with the release of this feature? I looked here and didn't see any results when I searched for upsertQueryData

[markerikson]

@blaine-foreflight : it's here:

https://deploy-preview-2401--redux-starter-kit-docs.netlify.app/rtk-query/api/created-api/api-slice-utils#upsertquerydata

I don't think Algolia indexes previews