useStableQueryArgs + defaultSerializeQueryArgs + serializeQueryArgs - avoiding attempts to serialize huge parameters

[fer662]

I've got a use case where a query takes a bunch of data as input. Like a lot. So since I don't want to end up with a cache key that's huge, I implemented serializeQueryArgs and provided a suitable replacement that uniquely represents the data in a manageable size string identifier with no collisions, perfect for a cache key. Imagine my surprise when i'm seeing long running attempts at memoizing a json the size of Texas and when i trace it I find this:

      const stableArg = useStableQueryArgs(
        options.skip ? skipToken : arg,
        // Even if the user provided a per-endpoint `serializeQueryArgs` with
        // a consistent return value, _here_ we want to use the default behavior
        // so we can tell if _anything_ actually changed. Otherwise, we can end up
        // with a case where the query args did change but the serialization doesn't,
        // and then we never try to initiate a refetch.
        defaultSerializeQueryArgs,
        context.endpointDefinitions[endpointName],
        endpointName,
      )

The comment feels like it's treating the user as dumb. Is there a way to avoid this?
In short: I have a parameter that i need in queryFn and I don't want the library to ever try to serialize it.

[phryneas]

The comment feels like it's treating the user as dumb.

We're not treating the user as dumb - if you look at the git blame, you see that this was introduced with #2844 to fix a user-reported bug in a specific valid usage scenario at #2816.

Or in other words: it's perfectly valid to want even multiple different queries to override each other in the same cache key sometimes, and then this logic is needed so you can switch between them.
I'm not sure how we can solve this more elegantly - do you have any suggestions?

[fer662]

Hey @phryneas thanks for the reply! First of all apologies if I sounded rude in my late night anger against the poor comment. Obviously you'll know all the possible uses cases where it'd matter better than me. I'll do my homework and read on those issues, but my first thought, assuming you agree my use case is worth considering, would be if it's possible to let the user override even this serialization at the endpoint level if they wanted to, or are you saying it'd never make sense to do that? My understanding of the contract with serializeQueryArgs is that I take on the responsibility of making sure i return a different value for 2 sets of parameters if they should be considered different. And I should clarify: i'm not changing the parameters and expecting the request not to trigger but rather my parameters are consistent, and i'm trying to save you the work of comparing them but you won't let me.

getSomething: builder.query<Something, GetSomethingArgs>({
      query: async ({queryArgs}) => {
        const result = await someRequestToGetSomething(queryArgs);
        return { data: result };
      },
      serializeQueryArgs: ({ queryArgs }) => {
        return `getSomething-${aReallyWellThoughtOutHashingFunctionForMyQueryArgs(queryArgs)}`;
      },
      yesIReallyMeanItPleaseUseMySerializationInternally: true,
      providesTags: (_result, _error, queryArgs) => {
        return [
          {
            type: "Something",
            id: aReallyWellThoughtOutHashingFunctionForMyQueryArgs(options),
          },
        ];
      },
    }),

Cheers!

[riqts]

I do recall that comment in the code is definitely aimed at the internal contributors feeling silly themselves, not trying to treat the users of the library dumb!

It does seem ultra niche to want to have it override the behaviour here in a way that just adding additional logic to the endpoint in the skip or queryFn couldn't solve.

I could see an easy implementation if this is considered a good idea being basically an extra flag like you suggested, or we expose the defaultQueryArgs function at a createApi level?

Another solution for your app's use case could be just using a fixed key and having a listener/middleware decide when the cache should be updated in response to state/serialisation changes?

[phryneas]

Hmm, tbh. I'm not 100% if we'd need to serialize in the first place - this could also be an option:

import { useEffect, useRef, useMemo } from 'react'
import { copyWithStructuralSharing } from '@reduxjs/toolkit/query'

export function useStableQueryArgs<T>(queryArgs: T) {
  const cache = useRef(queryArgs)
  const copy = useMemo(
    () => copyWithStructuralSharing(cache.current, queryArgs),
    [queryArgs],
  )
  useEffect(() => {
    if (cache.current !== copy) {
      cache.current = copy
    }
  }, [copy])

  return copy
}

In that case, defaultSerializeQueryArgs simply wouldn't play a role anymore.

[EskiMojo14]

would copyWithStructuralSharing not run into the same performance issues with large data?

[phryneas]

@EskiMojo14 depends on the data.
If it's 10 million objects with a hundred properties each, yes - but at least it won't consume much additional memory.
If parts of the object stay referential equal to a previous iteration it would save time again.

If it's just 500 objects that have absurdly long strings inside them that make the stringified version so big, it would be a very big win.

[markerikson]

      const stableArg = useStableQueryArgs(
        options.skip ? skipToken : arg,
        // Even if the user provided a per-endpoint `serializeQueryArgs` with
        // a consistent return value, _here_ we want to use the default behavior
        // so we can tell if _anything_ actually changed. Otherwise, we can end up
        // with a case where the query args did change but the serialization doesn't,
        // and then we never try to initiate a refetch.
        defaultSerializeQueryArgs,
        context.endpointDefinitions[endpointName],
        endpointName,
      )

The comment feels like it's treating the user as dumb. Is there a way to avoid this? In short: I have a parameter that i need in queryFn and I don't want the library to ever try to serialize it.

apparently I was the one who filed that PR and wrote that comment.

I'll be honest, this is a classic "wait, git blame says who wrote that?". I have no memory of doing so :) But I can safely say that the point here was that I had to spend some time figuring out what was going on, and was trying to brain-dump what I'd learned as a comment in case anyone ever had to look at this particular code (or the tweak) again.

[riqts]

I made a PR of what I understood @phryneas suggestion to be. Not sure if there are other aspects of it that need covering though, so happy to make changes there

[markerikson]

@fer662 could you try the PR preview build from #4996 and see if that helps your situation?

[fer662]

@fer662 could you try the PR preview build from #4996 and see if that helps your situation?

I'll try to give it a go sometime this week. I've basically worked it around using a WeakMap to pass around these huge objects since i already had unique identifiers for them.

@EskiMojo14 depends on the data. If it's 10 million objects with a hundred properties each, yes - but at least it won't consume much additional memory. If parts of the object stay referential equal to a previous iteration it would save time again.

If it's just 500 objects that have absurdly long strings inside them that make the stringified version so big, it would be a very big win.

In my case it's single level deep objects in the thousands with like 200 properties each. Do you think the proposed PR would help in this case?

[phryneas]

In my case it's single level deep objects in the thousands with like 200 properties each. Do you think the proposed PR would help in this case?

Depends of they are at least partially referential equal. Let's see.

[esauri]

I also came across this issue. In our case we pass an sdk client as a param to some of our queries.
We'd then use serializeQueryArgs to remove said param similar to the example in the docs.

However we got errors in the defaultSerializeQueryArgs called in useStableQueryArgs. We were able to resolve the issue by patching the changes in #4996