reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.72k stars 1.17k forks source link

Allow return of new object from upateQueryData instead of assuming/forcing mutation #2632

Closed AmyBlankenship closed 2 years ago

AmyBlankenship commented 2 years ago

We have patch endpoints that just take the properties to be patched. So the full object might look like:

{
    id: string  | number;
   prop1: string;
   prop2: string;
   prop3: string;
   prop4: string;
   //...
   propN: string;
}

the patch might be:
{
   id: 10,
  prop2: 'foo'
}

In most cases, I don't want to invalidate the tag for this, because it's a "parent", and just changing a prop on it shouldn't cause the whole tree to reload. So I'm doing optimistic updates, and only invalidating the tag if something happens that needs everything to reload. The immer architecture is forcing me to do this

/*  draft is an object that you have to mutate, so you have to go through
all the props of the incoming object and transfer them to the draft 🤦🏽‍
any's are bc lodash reduce still expects the param to be an array, even
though it can be an object (we are iterating through the keys) and
when you have any for first generic then typeof draft doesn't work 
*/
 reduce<any, any>(
   patchInfo.model,
   (draft, newValue, propName) => {
       draft[propName] = newValue;
    },
    draft
);

instead of [...draft, ...patchInfo.model]

But the signature of updateQueryData tells me I don't have any obvious way to use the simpler syntax.

markerikson commented 2 years ago

Sorry, not sure I'm following the situation here. Can you put together a CodeSandbox that shows this happening, particularly with some example TS types?

AmyBlankenship commented 2 years ago

I don't understand what would need to "happen", since the signature of updateQueryData is presumably something you know. It returns void, so even if I tried to return [...draft, ...patchInfo.model], there is nothing that will pick it up and do anything with it? Is there some secret overload that's not accounted for in the typing?

markerikson commented 2 years ago

I'm more asking to see what your app code looks like so I can better understand what you're trying to accomplish.

Like, you've got this reduce() there, but I don't know what the incoming data is, what the existing data is, how it needs to be applied, etc.

markerikson commented 2 years ago

The other thing is that I believe the callback you pass into updateQueryData ends up being passed straight to Immer. So, you should be able to either mutate draft, or return a new value, as usual with Immer.

The partial snippet in the first comment is also still confusing me - I think you left out too much context there, which is also why I'm asking for the sandbox.

AmyBlankenship commented 2 years ago

So let's say the existing data is

{id: 1
name: 'Frank'
job: 'Bricklayer'
}

And let's say that sometimes patchInfo.model is

{
  id: 1
  name: 'Jill',
}

other times it might be

{
   id: 1
   job: 'Astrophysicist',
}

etc.

Because it looks like you have to mutate the draft instead of returning a new value (and all of the examples I have looked at show that--none of them show returning a value), then it seems like you have no choice but to mutate the Immer draft.

If this is not the case, I guess that would be in the Immer docs, but TBH I feel like the chief value of immutability is in getting you to think in immutables, so a library that makes things immutable but encourages you to think about it as if it's mutable is something I don't have a huge interest in learning.

AmyBlankenship commented 2 years ago

Cool, just tried this and actually it does work even though I thought it would not from looking at the docs. Thanks!

AmyBlankenship commented 2 years ago

Also, I apologize for the typo where I did [...draft, ...patchInfo.model] instead of {...draft, ...patchInfo.model}. I can see now why that would be confusing.

markerikson commented 2 years ago

Yeah, the array-vs-object-spread thing was kinda throwing me off :)

But yes, in general, Immer has always let you either mutate the draft, or separately create and return a new immutably updated object yourself:

Inside updateQueryData, the internal logic is:

if ('data' in currentState) {
        if (isDraftable(currentState.data)) {
          const [, patches, inversePatches] = produceWithPatches(
            currentState.data,
            updateRecipe
          )
          ret.patches.push(...patches)
          ret.inversePatches.push(...inversePatches)
        } else {
          const value = updateRecipe(currentState.data)
          ret.patches.push({ op: 'replace', path: [], value })
          ret.inversePatches.push({
            op: 'replace',
            path: [],
            value: currentState.data,
          })
        }
      }

so if cacheEntry.data appears to be an object or array that can be mutated, we call Immer's produceWithPatches() and just immediately pass in your callback to do the work. So, the usual "mutate or return" rule applies.

AmyBlankenship commented 2 years ago

Thanks :). As I said, I don't really know what's usual for Immer. At some point I plan to write an article about my experiences with RTK Query, so this will all come to something that might be useful to someone. But my backlog is quite long.

BTW, you seem to be very quick answering questions. Do you have availability and would you be looking for work?

markerikson commented 2 years ago

Hah, I'm full time at https://replay.io, and very happy there :)

I just happen to be obsessive about refreshing unread notifications pay close attention to reported issues for both day job and Redux stuff.

AmyBlankenship commented 2 years ago

OK, my boss asked me to ask :-D

phryneas commented 2 years ago

Related reading recommendation: Writing Reducers with Immer - also applies to updateQueryData :)