Skip to content
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

RTK Query API pain points and rough spots feedback thread #3692

Open
markerikson opened this issue Sep 1, 2023 · 85 comments
Open

RTK Query API pain points and rough spots feedback thread #3692

markerikson opened this issue Sep 1, 2023 · 85 comments
Milestone

Comments

@markerikson
Copy link
Collaborator

It feels like the RTK Query API and options have a lot of rough edges (especially the goofiness around people trying to hack together infinite queries because we don't have anything built in for that right now, per #3174 and #1163 ), but I don't have more specifics in my head right now.

Please comment here to provide feedback about anything that annoys you with RTK Query's current API design and behavior!

Some items I know have been brought up:

  • The cache lifecycle methods do not run when there's a cache hit
  • There's a couple different cacheEntryRemoved/cacheDataLoaded promises, but you have to listen to both of them to do a finally {} equivalent
  • Kind of annoying to see all the someApi/fulfilled methods and not know the endpoint names (because it's buried in the meta field)
@markerikson markerikson added this to the 2.0 milestone Sep 1, 2023
@Dovakeidy
Copy link

First of all, I love RTK and RTKq! I use this library on many projects!

Currently, the real black spot for me is the infinite scroll management, as you mentioned. I've needed this feature several times and haven't found a solution that suits me perfectly.

So I've either switched to classic pagination, or I've used a not-so-great technique that consists of using pagination... but increasing the number of items to fetch. 20 items then 40 then 60 etc .
The result is infinite scrolling, but with an extra load for each request. However, it worked well in my case, given that there will never be thousands of data items.

Another very minor point is the data property on queries:

const { data, error, isLoading } = useGetPokemonByNameQuery('bulbasaur');

Every time I have to rename data:

const { data: pokemons, error, isLoading } = useGetPokemonByNameQuery('bulbasaur');

I think I would have preferred the same design as the mutations, so that I could directly name my datas with the name I want.

const [pokemons, { error, isLoading } ] = useGetPokemonByNameQuery('bulbasaur');

Right now I can't think of anything else, but I'll be back if I ever do!

@eric-crowell
Copy link
Contributor

eric-crowell commented Sep 2, 2023

Thank you for the forum.

Using a Single API for Various Libraries (w/ or w/o React)

My RTK Query APIs are commonly used in projects that don't use React; especially in testing environments. However, when a project does use an RTK Query API in an environment with React, I'd like to have a way to apply the hooks to it without having to re-create the API.

It would be excellent to have a pattern that implements React specific enhancements by passing it through a function...

import { reactify } from '@reduxjs/toolkit/query/react';
import { myApi } from '@mylib/apis/myApi';

export const myReactApi = reactify(myApi);
// myReactApi now has react hooks!

Or just have hooks that take an API/endpoint as an argument...

import React from 'react';
import { useQuery } from '@reduxjs/toolkit/query/react';
import { myApi } from '@mylib/apis/myApi';

export const myFC: React.FC = () => {
  // 
  const { data } = useQuery(
    myApi.endpoints.getSomething,
    { /* queryArgs */},
    { /* queryOptions */}
  );

  return <pre>{JSON.stringify(data, null, 2)}</pre>
}

@eric-crowell
Copy link
Contributor

eric-crowell commented Sep 3, 2023

Another suggestion for RTKQ.

Apply a Custom Action per Endpoint

(I don't think this capability exists without some major customizations. As far as I can tell it's not easily achievable. I understand there are a lot of powerful features in RTKQ, and some I might miss.)

Normally, I'm building Queries that acts on a state. The state's reducers/slices should know nothing of the Query APIs.

For example, I'd like to ask someone to build a RTK Query library for an API that populates our shared state (from a shared library) when invoked. I don't want to then go through all the Slices in our shared state library to add all those specific API matchers.

I want RTK Query API to dispatch the actions we already have.

That way, I can just plugin the RTK Query API in an application, invoke a query, and it populates the state how I want.

Rough Code Example

import { createApi, fetchBaseQuery } from "@reduxjs/toolkit/query";
import { myUpdateStuffAction } from '@shared/state/actions';

export const myApi = createApi({
  reducerPath: "myApi",
  baseQuery: fetchBaseQuery(),
  endpoints: (builder) => ({
    getStuff: builder.query({
      query: () => ({
        url: "/getStuff",
        method: "GET",
        action: myUpdateStuffAction,
        transformResponse: (response, meta, arg) => {
            const transformed = response.reduce(() => {
               /** Do some data transformation **/
            }, []);
            // The return must match the payload of the action property
            return transformed;
         }
      })
    })
  })
});

/**
 * The `myApi.endpoints.getStuff.initiate()` adds a property to the action telling me
 * which API & endpoint invoked it.
 * @example
 * {
 *    type: '@shared/myUpdateStuffAction',
 *    payload: { ... },
 *    apiFrom: '@myApi/getStuff'
 *    apiMeta: { ... }
 * }
 */

Other Thoughts

Honestly, in my mind, RTK Query is a tool to cleverly handle how to dispatch actions on a redux store when fetching data. I don't think it needs to expand my store with its own reducers and redux middleware. That information could be scoped inside itself.

That's my opinion though.

@nhayfield
Copy link

When generating from large OpenApi specs, it works pretty well. But there are issues if you want to enhance your endpoints, especially when using typescript. If you try to normalize your endpoints then it will lose the correct type and throw typescript errors throughout your application.

Also after generation the api file, it does not seem to be easy to add additional methods to consolidate multiple calls by modifying the generated file.
It is also not easy to do other custom things like access response headers from the generated file. It seems like generating from open api spec is actually a mistake and you really need to just write a bunch of code manually to use any of the good features.

Maybe I am wrong about some of this. Feel free to correct me.

@markerikson
Copy link
Collaborator Author

@nhayfield can you give a couple further details?

  • What do you mean by "normalize the endpoints"?
  • What's an example of "consolidating multiple calls"?
  • Where and how would you want to access response headers?

@nhayfield
Copy link

nhayfield commented Sep 6, 2023

@nhayfield can you give a couple further details?

  • What do you mean by "normalize the endpoints"?
  • What's an example of "consolidating multiple calls"?
  • Where and how would you want to access response headers?

sorry by normalize just meant reindexing by id for faster lookups on the list type queries.

consolidating calls i meant for calls that depend on one another it is better to chain them together.

i would like to access response headers from the generated hooks.

all of these are possible when building the routes from scratch. but they become fairly difficult when generating from an openapi spec and especially when using typescript

@markerikson
Copy link
Collaborator Author

@nhayfield can you show a concrete example of what a handwritten version of this looks like? I get the general sense of what you're saying, but I need more details to get a better sense of what the pain points are and what possible solutions we might come up with.

I'm assuming that normalizing is something you would typically do with transformResponse.

Where and how would you want to "chain calls together"?

Where and how would you want to access response headers, in what code?

@jarvelov
Copy link

jarvelov commented Sep 7, 2023

Just wanted to chime in and express support for the proposal of unifying the API by @Dovakeidy:

I think I would have preferred the same design as the mutations, so that I could directly name my datas with the name I want.

const [pokemons, { error, isLoading } ] = useGetPokemonByNameQuery('bulbasaur');

I know this is not a change one makes lightly and understand the considerations one has to make before changing an API. However, this proposal makes immediate sense to me and the different APIs for Queries/Mutations has been a source of confusion in my team.

@nhayfield
Copy link

@nhayfield can you show a concrete example of what a handwritten version of this looks like? I get the general sense of what you're saying, but I need more details to get a better sense of what the pain points are and what possible solutions we might come up with.

I'm assuming that normalizing is something you would typically do with transformResponse.

Where and how would you want to "chain calls together"?

Where and how would you want to access response headers, in what code?

#3506
#3485
these are the issues, regarding the issues with enhanceEndpoints, transformResponse, and typescript.
it doesn't seem like there has been any movement on the PR that addresses the issue.

const { data, error, isLoading } = useGetPokemonByNameQuery('bulbasaur');
this is an example of the type of generated hook i am wanting to access the Response Headers from. It doesn't appear possible.

@markerikson
Copy link
Collaborator Author

markerikson commented Sep 7, 2023

@nhayfield : I still don't think I understand where in that query hook output you would expect to find and access the response headers. Something like {data, isLoading, headers} ?

It's important to remember that RTKQ, at its core, doesn't even know about HTTP at all. It just tracks some kind of async request's status, and the async function is supposed to return an object like {data} or {error}. None of that is HTTP-specific. It's fetchBaseQuery that makes an HTTP request specifically. So, conceptually, headers don't fit into the output format of a query hook, because nothing about that result relates to HTTP at all.

I think you might want to try writing a custom version of fetchBaseQuery that includes the headers as part of whatever actual data value was fetched, so that they'll get saved into the cache entry:

@nhayfield
Copy link

@nhayfield : I still don't think I understand where in that query hook output you would expect to find and access the response headers. Something like {data, isLoading, headers} ?

It's important to remember that RTKQ, at its core, doesn't even know about HTTP at all. It just tracks some kind of async request's status, and the async function is supposed to return an object like {data} or {error}. None of that is HTTP-specific. It's fetchBaseQuery that makes an HTTP request specifically. So, conceptually, headers don't fit into the output format of a query hook, because nothing about that result relates to HTTP at all.

I think you might want to try writing a custom version of fetchBaseQuery that includes the headers as part of whatever actual data value was fetched, so that they'll get saved into the cache entry:

doesnt matter where, as long as it could be accessed. could be metadata or headers. not sure the basequery is an option because these are the response headers instead of the request headers.

@rwilliams3088
Copy link

From a usability perspective, there are two big draw-backs for me right now.

First is the lack of official support for complex objects as inputs and outputs of an api endpiont. I have been able to work around it by turning off the serialization warnings and by taking advantage of the transformServerResponse callback, but official support for both serializing the endpoint arguments and for deserializing the response would really polish up the library.

The second major thing is the bug around mutations and caching. If you mutate data and then attempt to refetch it immediately afterwards - if there was a pending request to fetch the data before the mutation, then the subsequent re-fetch erroneously returns the old cached data :/ This has prevented me from being able to take advantage of the caching features of this library

@markerikson
Copy link
Collaborator Author

@rwilliams3088 can you clarify what you mean by "lack of official support for complex objects? What's an example of that?

@rwilliams3088
Copy link

rwilliams3088 commented Sep 11, 2023

@rwilliams3088 can you clarify what you mean by "lack of official support for complex objects? What's an example of that?

For example: a Date object. I use a number of these throughout my API. By default, if you attempt to pass Date objects into or out and Api Endpoint, you are going to get errors from the serialization check - since, of course, you aren't supposed to pass object references via redux.

It would be very inconvenient to make the user of an endpoint serialize all the data themselves before being able to use the endpoint. And it could be error-prone as well. For a complex object like a Date, the format that gets sent to the server may change for different endpoints. Most will be ISO8601 of course, but some of them may only want the date component, some may require timezone adjustments, etc. Similarly, when I get a Date back from the server, I want it deserialized back into a Date then and there - and I may want to perform a timezone adjustment as well (UTC => local time).

So some basic configuration options for serialization and deserialization on the way into and out of redux would make things a lot smoother and not require work-arounds. You could name the serialization parameter reduxify 💯

@rwilliams3088
Copy link

rwilliams3088 commented Sep 11, 2023

Another, smaller request for efficiency: drop uneccessary state like result and lastPromiseInfo from the useLazyQuery hook. Since trigger returns a promise with all the request and response details, and since I'm often sending many simultaneous requests, that state holds little value to me. Furthermore, it means that each time I send a request there are unnecessary re-renders of the component using the hook.

@Dovakeidy
Copy link

@rwilliams3088 Generally, if you're redoing a lazy query, you want to have a re-render to get the new query data from the hook. I don't really see the problem ?

@seanmcquaid
Copy link

seanmcquaid commented Sep 11, 2023

Personally, I would love to have the ability to have an onSuccess/onError callback options for hooks for Mutations! Tanstack/React Query offers this and it's quite nice.

@markerikson
Copy link
Collaborator Author

@seanmcquaid What's the benefit of having callbacks as opposed to using await doSomeMutation()?

(note that React Query is removing its callbacks for queries in the next major, but apparently not for mutations? https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose )

@seanmcquaid
Copy link

@markerikson - Thank you for the insanely quick reply, you are the best!

Good callout on mentioning that they're removing this from Queries and not mutations, that's why I only mentioned this for mutation hooks.

I think it personally reads a bit better when you remove that async handling with mutations and can essentially just move that logic into an object provided to the hook itself. Instead of potentially needing try/catch in a different function for it. Just a preference!

@rwilliams3088
Copy link

rwilliams3088 commented Sep 11, 2023

@rwilliams3088 Generally, if you're redoing a lazy query, you want to have a re-render to get the new query data from the hook. I don't really see the problem ?

Once I get the data, yes I'll probably want to re-render - but I don't need to re-render at the time that a request is submitted, when the args change, which will occur prior to receiving the data. Nor do I want a re-render as the request goes through intermediate state changes. Also, in the case of multiple requests getting fired off - some of them maybe cancelled (for example: when filters/sorts change on the front-end such that previous requests are now irrelevant), so I don't need to re-render at all for those requests.

It's not the end of the world if there are extra re-renders, but they are also completely unnecessary. One can add their own lastArgs state to their component easily enough if they are really interested in tracking it.

@mjwvb
Copy link

mjwvb commented Sep 12, 2023

I'm a very happy user of RTK query for a very data intensive desktop app. Some feedback off the top of my head:

  • Invalidate current cache entry from within onCacheEntryAdded (e.g. invalidateCachedData, just like updateCachedData). Right now we use some tag invalidation trickery for that.
  • Invalidation of long running queries (Invalidating long running initial GET requests. #2215). Say you have a long query of 10 seconds and you invalidate during that request, it won't cancel that request and won't trigger a new request.
  • Ability to cancel queries in general.
  • Maybe an easier way of passing extra options/parameters to a query without making it part of the query args. Since serializeQueryArgs it is possible to omit arguments from serializing, but it's a little bit cumbersome.

@markerikson
Copy link
Collaborator Author

markerikson commented Sep 12, 2023

@mjwvb : thanks! A number of folks have mentioned the idea of "canceling queries". Can you describe what you would expect to happen in that case?

Also, what's the use case for invalidating individual entries?

@mjwvb
Copy link

mjwvb commented Sep 12, 2023

I think cancelling should abort the running promise for a given endpoint in two possible ways: Locally using an abort function as returned by e.g. useQuerySubscription, and globally by using tags in the same way as invalidateTags. The endpoint entry should then return an error state with a "cancelled" error code, in which I will be responsible to refetch. When it is cancelled after a refetch from invalidation: just cancel that request and keep the cached data.

Our (simplified) use case for invalidating individual entries is a little bit more niche though, and maybe another pain point in itself. We have data grids in which the user is able to add more data columns after the rows have been loaded. We want the new columns to be fetched incrementally instead of refetching all columns again. Initially we thought serializeQueryArgs with forceRefetch could help us out here, but in the end it wasn't possible. We came up with a complicated solution in which the visible columns are tracked in a global class outside the endpoint, linked using some sort of ID. Then in onCacheEntryAdded we listen for a visibleColumnsChange event and then try to fetch the extra columns. When the fetch request for the new columns has failed, we simply invalidate that cache entry so it will refetch all rows for all the visible columns. That's when a invalidateCacheEntry would be nice to have :).

Sounds way too complicated, however we already had the class instance in place for other purposes so it was relatively easy to implement. Anyway besides invalidateCacheEntry, I think the incremental fetching of data is a rough spot on its own.

@mjwvb
Copy link

mjwvb commented Sep 12, 2023

Now that I think about it, I'm unsure why serializeQueryArgs/forceRefetch/merge didn't provide the solution... Theoretically it should be possible if I'm not mistaken? Our complicated implementation was before the availability of serializeQueryArgs etc., so it was already working and not high on the prio list to be refactored. Gonna look into it again tomorrow.

@markerikson
Copy link
Collaborator Author

markerikson commented Sep 18, 2023

Tossing out a few things that I know have come up a number of times:

  • There's no immediate way to do "dependent queries" via just the core API. The only real workaround is a custom queryFn that dispatches the first request thunk, and then use that result for the second query. Conceptually, this feels sort of like allowing queries to invalidate tags too?
  • We currently implement the React hooks behavior by having an entire second implementation of the RTKQ createApi method with the hooks logic mixed in. It would be nice if we could do it as more of a layer on top of the core API. In other words, create a UI-less api instance first, then add the hooks on top of that. The basic public usage would still be the same, just import { createApi } from "@reduxjs/toolkit/query/react", but this would allow generating a UI-less api instance for use on the server side, and then extending it with the React hooks on the client side. It might also simplify other UI integrations as well. (Conceptually, I can almost envision it as a form of injectEndpoints - call methods on the original API instance, return the same API instance but with new capabilities built in and updated TS types?)
  • Infinite queries ala Implementing infinite scroll with RTK Query #1163 and RTKQ "infinite query" potential use cases and concerns #3174
  • A pseudo-normalization approach. If you fetch a list of items, it would be neat to also add those to the store as individual entries as well (so, getTodos() could also save items as getTodo(todoId)). I feel like that could help bridge the gap with some more complex use cases. (Some interesting discussion here: https://twitter.com/bancek/status/1703880605379784832 )

@codingedgar
Copy link

I struggle with cache and optimistic updates, specially when I have:

fetchAllOfX -> Saves in one cache
CrudOfX -> optimistic update/cache invalidation to fetchAll might not work because I cannot add tags while updating cache, so added entries are “imposible to invalidate”

wish I could normalize the cache or customise it in some way that allows to share ir between URLs

@agusterodin
Copy link

agusterodin commented Sep 19, 2023

Would love a way to reset the data in a useQuery hook. This would be helpful for for autocomplete searchboxes in particular.

Screen.Recording.2023-09-18.at.10.58.47.PM.mov

When the user clicks an item in the autocomplete dropdown, I reset the search query to a blank string. Since I have {skip: searchboxText === ""}, the "data" doesn't reset to blank. As soon as the user goes to use the searchbox again it immediately shows the old data from the previously entered search term.

Not sure if this is helpful but here is rough code on how i'm using it


export default function TargetListFormSearchbox() {
  const dispatch = useDispatch()
  const searchboxText = useSelector(getTargetListFormSearchboxText)

  const debouncedSearchboxText = useDebouncedSearchboxText(searchboxText, 750)
  const { data: rawSearchResults } = useGetTargetListSearchResultsQuery(debouncedSearchboxText, {
    skip: searchboxText === null
  })
  const searchResults =
    searchboxText !== '' ? rawSearchResults?.map(searchResult => ({ id: String(searchResult.id), value: searchResult.name })) || null : null

  const inputRef = useRef<HTMLInputElement>(null)

  const form = useTargetListForm()

  return (
    <AutoCompleteSearchbox
      inputRef={inputRef}
      className="-ml-2.5 -mt-2 mb-2 w-[310px]"
      placeholder="Search existing target lists"
      results={searchResults}
      value={searchboxText}
      setValue={value => dispatch(setTargetListFormSearchboxText(value))}
      onResultSelected={result => {
        inputRef.current?.blur()
        dispatch(setTargetListFormSearchboxText(''))
        dispatch(fetchTargetList({ id: parseInt(result.id), targetListForm: form }))
      }}
      onClear={() => dispatch(setTargetListFormSearchboxText(''))}
    />
  )
}

The only viable workaround I found is to use queryFn

import { campaignsApi } from './index'

const targetListApiSlice = campaignsApi.injectEndpoints({
  endpoints: builder => ({
    getTargetListSearchResults: builder.query<null | TargetListSearchResult[], string>({
      queryFn: async (searchText, baseQueryApi, baseQuery, fetchWithBaseQuery) => {
        if (searchText === '') return { data: null }
        return (await fetchWithBaseQuery(`/targetList/search/${searchText}`)) as any
      },
      keepUnusedDataFor: 0
    })
  }),
})

export const { useGetTargetListSearchResultsQuery, endpoints: targetListEndpoints } = targetListApiSlice

This tricks the hook into resetting the data of the hook to null when searchbox is empty. Unfortunately the type-safety isn't ideal.

I thought of using currentData instead, but that would make it so the dropdown doesn't "smoothly" change between results (result dropdown will periodically disappear every time a character is typed)

I also tried using resetApiState directly after a result is selected. This doesn't reset the hook state. This aligns with what the docs say:

"Note that hooks also track state in local component state and might not fully be reset by resetApiState."

I can provide a reproduction if necessary but figure this is already an acknowledged behavior as shown in the docs.

@xjamundx
Copy link

  1. This might be a bit out of scope, but I initially found it incredibly challenging to see how RTK Query interfaced with my redux store. Even now it feels very secret and separate and that makes migrations quite awkward. If you're starting from scratch and don't have pre-conceived notions of how redux works, it's fine, but if you're trying to adopt RTK Query into an existing redux application, it can be a bit weird. What's the takeaway here? IDK.
  2. graphql works, but it's poorly documented and arguably a bit convoluted. I initially tried it, got it working awkwardly then switched to urql, before the team pressured me to switch to apollo. I still like RTKQ best for many things, but a full effort to make graphql support first-class would be great. It could become a real player in graphql clients potentially.
  3. I'm still not convinced a full API-first approach is needed and that a simpler API might be possible that's more built around URLs rather than APIs. Just thinking about making getting starting easier.

@markerikson
Copy link
Collaborator Author

@xjamundx can you give more examples of each of those?

For the "migration" aspect, does https://redux.js.org/usage/migrating-to-modern-redux#data-fetching-with-rtk-query help at all? What info would be more useful here? What aspects about the "interfacing" are confusing?

@phryneas
Copy link
Member

phryneas commented Nov 29, 2023

@rjgotten you don't have to do 100% of your work inside only one of those functions.
Your use case is more complicated, so you'll need to do a bit more work - but it's still perfectly possible. You get all the signal/information you need from those two lifecycle events, and you can combine them.

Here is some pseudocode to outline this - nothing more concrete, because it's late at night and I'm really tired:

const openWebsockets = Map<cacheId, WebsocketInstance>()

onCacheEntryAdded(cacheId, cacheRemoved ){
  openWebsockets.set(cacheId, new WebSocketInstance(someInformation)
  await cacheRemoved;
  openWebsockets.get(cacheId).close()
  openWebsockets.delete(cacheId)
}

onQueryStarted(cacheId, queryFulfilled) {
  const latestDataFromQuery = await queryFulfilled
  openWebsockets.get(cacheId)?.connectOrReconnect(latestDataFromQuery)  
}

To be honest, in more complex cases you'll likely end up with something like this anyways, probably you even only want one shared websocket connection for all your cache entries instead of one per cache entry, and just send/receive messages on that connection from all the lifecycle events from all your cache entries.

@rjgotten
Copy link

rjgotten commented Nov 30, 2023

@phryneas
Ok. That's clever actually.

One remaining question then:
in the event of a resetApiState call, are the cacheRemoved promises all resolved and the onCacheEntryAdded callbacks all allowed to properly unwind?
Or would that leak the cacheIDs and connections held in the Map - i.e. would there need to be some additional leg work to ensure that the connections are closed and cleaned up for that case?

@phryneas
Copy link
Member

@rjgotten yes, all of them will be called:

} else if (api.util.resetApiState.match(action)) {
for (const [cacheKey, lifecycle] of Object.entries(lifecycleMap)) {
delete lifecycleMap[cacheKey]
lifecycle.cacheEntryRemoved()
}
}

@nikischin
Copy link

nikischin commented Dec 4, 2023

Thank you for your amazing tools provided with RTK!
It would be super cool to have some kind of debounced/lazy/autosave mutation.

What it should actually do:

  • Register API call for a mutation but not trigger it
  • If another call for same endpoint is registered, override previous one
  • If no new call is registered for a certain amount of time at this endpoint, submit API call. Probably also give the user the option to force updates at a defined interval of time.
  • Callback onMutationRegistered like onQueryStarted so the user can perform some (very) optimistic updates logic for cache manipulation.

Where would we need something like this:

  • On a text editor or some user forms which should be saved in regular intervals and not only on submit.

I implemented my own implementation for it which is quite messy, however, this might give you an idea:

function buildAutoSaveHook (endpointName) {
    return ()=> {
        const useQueryMutation = (generalApi)[`use${upperFirst(endpointName)}Mutation`];
        const [ mutation, ...mutationFunctions ] = useQueryMutation();
        const [ queryData, setQueryData ] = useState();
        const [ queryOptions, setQueryOptions ] = useState();
        const [ patchResult, setPatchResult ] = useState([]);
        const { dispatch, getState } = useStore();

        const onSave = async (concatinatedQueryData)=> {
            if (concatinatedQueryData !== undefined) {
                // Reset Very optimistic updates - mutation will now further handle it...
                patchResult.forEach(res=> {
                    res?.undo();
                });

                mutation(concatinatedQueryData, queryOptions);
            }
        };

        useAutosave({ data: queryData, onSave: onSave });

        return [ (data, options)=> {
            setQueryData(data);
            setQueryOptions(options);

            setPatchResult(optimisticEndPointUpdates[endpointName]?.map((funcs)=> {
                const optimisticDefParams = funcs(data, getState);

                if (optimisticDefParams) {
                    return dispatch(generalApi.util.updateQueryData(...optimisticDefParams));
                }
            }) ?? []);
        }, ...mutationFunctions ];
    };
}
//Add auto generated AutoSaveMutation Endpoints
Object.values(generalApi.endpoints).forEach(endpoint=> {
    if (endpoint.useMutation) {
        const useAutoSaveMutation = buildAutoSaveHook(endpoint.name);
        Object.assign(generalApi.endpoints[endpoint.name], useAutoSaveMutation);
        (generalApi)[`use${upperFirst(endpoint.name)}AutoSaveMutation`] = useAutoSaveMutation;
    }
});

@dhlavaty
Copy link

dhlavaty commented Dec 7, 2023

It would be super useful to be able to access QueryArg used in queries and mutations in prepareHeaders(). And/or be able to send some arbitrary context object from queries/mutations to prepareHeaders().

It will allow (or hugely simplify) the implementation of OAuth's Resource and Scope Aware Authorization for example.

And example was mentioned at #3920

@chriskuech
Copy link

I can't tell if we're fundamentally missing something about refetches, but way too often we have to deal with this error coming up in the app:

Error: Cannot refetch a query that has not been started yet.

  • We use api.util.invalidateTags to invalidate the cache without refetch, but that doesn't eagerly refetch existing queries on the page.
  • I have tried many things to see if refetch shouldn't be called--checking for isUninitialized, data, etc.--but still get this error. At this point I'm just going to handle/ignore the errors when I prematurely refetch.

@phryneas
Copy link
Member

phryneas commented Dec 7, 2023

@chriskuech that sounds like you are using a stale reference to the refetch function of the first render, instead of using the refetch function of a current render. Are you missing refetch in a dependency array?

@micmcg
Copy link

micmcg commented Dec 8, 2023

I have a basic cursor paged query set up using a custom serializeQueryArgs to exclude the paging args from the cache key and a custom merge to append the new results to the existing results. All this is working fine.

My problem is when trying to use tag invalidation. Say a mutation modifies this paged list by adding or removing an item. This invalidates the provided tag on the paged list (as it should)

The problem is what happens next.
https://github.com/reduxjs/redux-toolkit/blob/master/packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts#L86-L125

Specifically

if (querySubState.status !== QueryStatus.uninitialized) {
  mwApi.dispatch(refetchQuery(querySubState, queryCacheKey))
}

So what happens is that because there is already state in the cache, it calls the query again with the same arguments as the last call (include the pageToken). This just refetches the last page (and then merge adds it to the end of the existing results, resulting in a page of dupes and no update to the actual changed data).

What I want is to be able to detect when the query is being refetched as the result of tag invalidation (and not due to say useLazyQuery's trigger function) so I can ignore the pageToken being passed in, and instead make the request with no pageToken (requesting the first page again). If I can do that, I can probably add some handling in merge to handle the case when the pageToken arg is undefined, and overwrite the cache with the new page rather than appending it.

However I have tried all different approaches and I am not able to isolate the refetch case. Using queryFn I can see that forced is true in the refetch case, but it is also true when using trigger from useLazyQuery. I've tried comparing the incoming query args to the originalArgs stored in the state to check if it is a duplicate/refetch request, but there is no nice way to access the existing query state from the queryFn (api.getState() returns the entire redux store, so navigating to the state for this query either requires a lot of knowledge about the reducer path and cache key, or is not possible in the case of a helper function for building paged endpoints.)

EDIT: After looking at the above further, I can see that it is not possible to detect duplicate requests in the queryFn by looking at the existing state as originalArgs has already been updated to the new args by the point the queryFn is executed (and therefore originalArgs would always be equal to the incoming query args, giving a false positive for a duplicate request)

EDIT EDIT: Actually, because in my case the pageToken is stored in the data part of the query cache, I can check and see if the pageToken arg is different to the pageToken in data.pageToken. If they are same, it is a normal request for the next page. If they are different it is a refetch of the current page. However I still don't have a good way to access the cache entry in queryFn. onQueryStarted gives us api.getCacheEntry() which would be ideal. Alternatively, if I could abort the request from onQueryStarted I could cancel the request and reset the cache with updateCachedData. Still I am very much open to a better solution if you have one.

Can you please advise if there is a way to solve this issue I am having?

@micmcg
Copy link

micmcg commented Dec 8, 2023

If you are interested in my code, this is the helper function we use for building paged endpoints and the helper function for building a hook to simplify fetching the next page from the endpoint

/**
 * Build a query for fetching paged data.
 * By sharing the cache key between pages, and appending the new data to the old rather than overwriting it,
 * the single cache entry will contain all the fetched pages of data.
 *
 * Most of the time you will not call the generated hooks for this endpoint directly, instead use `buildUsePagedQuery`
 * to create a hook that will fetch the first page and provide a `fetchNextPage` function to fetch subsequent pages,
 * abstracting away the handling of the pageToken.
 *
 * @param builder - The endpoint builder to use
 * @param requestBuilder - A function that takes the query args and returns an AxiosRequestConfig for the request (url, params etc)
 *
 * @example
 * endpoints: builder => ({
 *  getUsers: buildPagedQuery<User, UsersFilterParams>(builder, params => ({ url: baseUrl, params })),
 * })
 *
 * const usePagedUsersQuery = buildUsePagedQuery(usersApi.endpoints.getUsers)
 *
 * const { data, isFetching, fetchNextPage, allFetched } = usePagedUsersQuery()
 *
 * @returns The query definition
 */
export const buildPagedQuery = <DataType, QueryArgType = void>(
  builder: AxiosEndpointBuilder,
  requestBuilder: (params: Omit<WithPageArgs<QueryArgType>, 'pageToken' | 'limit'>) => AxiosRequestConfig,
  extraOptions?: AxiosQueryEndpointExtraOptions<PagedData<DataType>>
) =>
  builder.query<PagedData<DataType>, WithPageArgs<QueryArgType>>({
    query: ({ pageToken, limit, ...params }) => {
      const requestConfig = requestBuilder(params)

      return {
        ...requestConfig,
        params: { limit, pageToken, ...requestConfig.params },
      }
    },
    serializeQueryArgs: endpoint => {
      return defaultSerializeQueryArgs({
        ...endpoint,
        // Don't serialize the pageToken or limit into the cache key so subsequent pages are added to the existing cache entry
        queryArgs: omit(endpoint.queryArgs, 'pageToken', 'limit'),
      })
    },
    merge: (currentData, nextPageData, args) => {
      if (!Array.isArray(currentData.data)) {
        currentData.data = []
      }
      if (Array.isArray(nextPageData.data)) {
        currentData.data.push(...nextPageData.data)
      }
      currentData.pageToken = nextPageData.pageToken
    },
    ...extraOptions,
  })

/**
 * A function that can be used to build a query hook that will fetch the first page of results from a paged endpoint
 * (i.e. one created with `buildPagedQuery`) and provide a `fetchNextPage` function to fetch the next page of results.
 *
 * `fetchNextPage` will not attempt to fetch any more pages if the last page has already been fetched.
 * (i.e. if the `pageToken` returned by the last request was `null`).
 * An additional `allFetched` boolean is provided to indicate if all pages have been fetched.
 *
 * @example
 * const usePagedUsersQuery = buildUsePagedQuery(usersApi.endpoints.getUsers)
 *
 * const { data, isFetching, fetchNextPage, allFetched } = usePagedUsersQuery()
 *
 * <Button disabled={isFetching || allFetched} onClick={fetchNextPage}>Load more</Button>
 */
export const buildUsePagedQuery =
  <DataType>(pagedEndpoint: AxiosQueryEndpoint<PagedData<DataType>>) =>
  (params = {}): PagedQueryState<DataType> => {
    const useQueryResponse = pagedEndpoint.useQuery(params)
    const pageToken = useQueryResponse.data?.pageToken
    // Strict equality check to ensure that pageToken is null and not undefined
    // (which would indicate that the query has not been run yet)
    const allFetched = pageToken === null

    const [triggerQuery] = pagedEndpoint.useLazyQuery()

    const requestInFlight = useQueryResponse.isLoading || useQueryResponse.isFetching

    const fetchNextPage = useCallback(() => {
      if (!allFetched && !requestInFlight) {
        triggerQuery({ pageToken, ...params })
      }
    }, [allFetched, requestInFlight, triggerQuery, pageToken, params])

    return {
      ...useQueryResponse,
      data: useQueryResponse.data?.data, // Only return the data, not the pageToken
      currentData: useQueryResponse.currentData?.data,
      fetchNextPage,
      allFetched,
    }
  }

@micmcg
Copy link

micmcg commented Dec 12, 2023

So I have a solution for the above largely working, except that looking up the existing cache state in queryFn is really messy.

If I could ask one thing, it would be to make api.getCacheEntry() which is available in onQueryStarted available in queryFn also.

That would remove the need to know the reducerPath and cache key in queryFn and would tidy up my code significantly

Thanks

@nikischin
Copy link

@micmcg

Talking about paginated queries, here's another sample of another implementation, I just found while looking up other stuff: #1072 maybe this does help

@padge
Copy link

padge commented Dec 13, 2023

Another use-case for @eric-crowell's "reactify" an api idea, is that many times I want to use the useQuery hooks in my components, but other times I need to refetch an api query in one of my sagas, or run a mutation in a saga that breaks the query cache, etc.

@EskiMojo14
Copy link
Collaborator

@padge what's preventing you from doing that currently? the version of createApi from query/react still has everything that the version from query has.

@padge
Copy link

padge commented Dec 13, 2023

@EskiMojo14 my mistake, I somehow missed the "Usage Without React Hooks" page and initialize. I was expecting something like api.endpoints.getFoo.query() or similar (new to rtk-query). I'll look into it – thanks.

@ivp-dev
Copy link

ivp-dev commented Dec 20, 2023

@micmcg Thank you for sharing your code. I implemented it with some minor changes and encountered the exact issue you described above. The only solution I can think of is to create a fetchFirstPage callback in buildUsePagedQuery and use this callback to invalidate the cache where needed. It's not a perfect solution, but I can't see any other alternatives.

@Richard87
Copy link
Contributor

Richard87 commented Jan 9, 2024

Hi!

Also, we only use code-generation, and would love to specifiy global options in createApi(), specifically pollingInterval instead of setting it on every useQuery hook

@markerikson
Copy link
Collaborator Author

@Richard87 looks like you edited the message, but to answer your earlier question: mutation triggers are standard RTK createAsyncThunks, so you can do await someTrigger.unwrap() to convert that back into a thrown error:

@leafty
Copy link

leafty commented Jan 24, 2024

One of the biggest pain points I have encountered is dealing with the fact that the useQuery hook will fetch data on mount when there was a previous useQuery hook which resulted in an error state. This has often created infinite render loops when two components use the same query and one is nested inside the other.

Example:

function ComponentA() {
  const { data, isLoading, isError } = useMyQuery();
  
  if (isLoading) { return <p>Loading...</p> }
  return (
    <div>
      <h3>{isError ? "Error" : data.title}</h3>
      <ComponentB />
    </div>
  );
}

function ComponentB() {
  const { data, error } = useMyQuery();
  if (error) { return <p>{error.message}</p> }
  return (<p>{data.message}</p>);
}

@rjgotten
Copy link

rjgotten commented Jan 24, 2024

One of the biggest pain points I have encountered is dealing with the fact that the useQuery hook will fetch data on mount when there was a previous useQuery hook which resulted in an error state.

I've actually experienced more or less the inverse of that as a problem:
if there's a useQuery hook subscribed; its args change and the query re-runs, but hits an error state, it retains the data from the last succesful query even though that had totally different args.

Basically, it means you need to either always use currentData instead of data and not care.
OR you need to pedantically check isError and error all the time.
(Humorous observation: it means you can have data and error at the same time. Bit of an identity crisis, no?)

@EskiMojo14
Copy link
Collaborator

@rjgotten this is documented:

  • data - The latest returned result regardless of hook arg, if present.
  • currentData - The latest returned result for the current hook arg, if present.

@rjgotten
Copy link

@EskiMojo14

Documented behavior or not - it's a pain point.
Also, I would argue that the documentation is not being very clear there.

It does not firmly state that data will hold the latest succesful returned result regardless of hook arg.

I'd also argue that the behavior as-is, is strange.
I would expect data to hold on to the last returned result regardless of hook arg, until any latest running query's finish running and if those fail for data to become undefined like currentData would.

Basically, if considering the query as a promise I would expect data (and for that matter maybe also error ?) to 'pin' their last settled values, whether resolved or rejected, while the promise is still pending, but to always accurately reflect the latest settled value, again: whether resolved or rejected.

Orthogonality and principle of least surprise apply.

@phryneas
Copy link
Member

phryneas commented Jan 24, 2024

I would expect data to hold on to the last returned result regardless of hook arg, until any latest running query's finish running and if those fail for data to become undefined like currentData would.

Imagine a UI that displays a list of elements, currently on page 1. You navigate to page 2, so page 1 stays in view, but grays out (to prevent the UI from jumping around). Now you encounter an error.
You might want to present a pop-up with the error message and a "refetch" button, but at the same time keep the old (still grayed-out) data visible in the background. That data isn't wrong, it's just not current - and it's the last successful state your application has.

Something like that would prevent a janky UI jumping from one state into another and creating a lot of motion - and it's only possible if the last successful value stays available to you in some way.

@amirghafouri
Copy link

Imagine the following pattern:

I have hooks such as useFetchTrendingPostsQuery, useFetchFeaturedPostsQuery, useFetchTechnologyPostsQuery, etc... which all return different entities of the same type.

I have a Post component, which I want to accept only a postId prop.

There is a useFetchPostQuery which would accept a postId param and fetch a single post.

However, I don't want to trigger this query if the post has already been fetched by one of: useFetchTrendingPostsQuery, useFetchFeaturedPostsQuery, useFetchTechnologyPostsQuery.


I can't really use selectFromResult, because Post is a generic component and it would have to check all of the different api queries for different types of posts.

I think I could use extraReducers to put the results from the various query hooks for different types of posts into a single store slice called posts and check that, but that feels like it defats the point of having rtk-query?


One thing I could do is provide tags from the results of those queries, such as:

fetchFeaturedPosts: builder.query({
      providesTags: data => {
          return data.map(i => ({ type: 'post', id: i.id }));
      },
      query: fetchFeaturedPostsQuery
}),
fetchTrendingPosts: builder.query({
      providesTags: data => {
          return data.map(i => ({ type: 'post', id: i.id }));
      },
      query: fetchTrendingPostsQuery
}),
fetchPost: builder.query({
      providesTags: (data, error, arg) => {
          return [{ type: 'post', id: arg.postId }];
      },
      query: fetchPost
}),

I would just need a way to link the {type: 'post', id: postId } tag back to useFetchPost in a way that would make it avoid fetching that specific post if the postId matched.

Essentially, RTK-Query uses a document cache by default, but via tags I think users could have a handy way to manually create normalized relationships between the different queries by processing the data results in providesTags.

@rjgotten
Copy link

rjgotten commented Feb 15, 2024

@amirghafouri
Use manual cache population.

Hook into onQueryStarted on each of your endpoints which fetches collections of posts, await queryFullfilled so you have the result, and then push that into a manually created or updated cache entry for fetchPost.

You will probably also need to make sure that fetchPost cache has a lifetime that doesn't immediately invalidate when it has no subscriptions.

@tibo-glamarche
Copy link

Tossing out a few things that I know have come up a number of times:

  • There's no immediate way to do "dependent queries" via just the core API. The only real workaround is a custom queryFn that dispatches the first request thunk, and then use that result for the second query. Conceptually, this feels sort of like allowing queries to invalidate tags too?
  • We currently implement the React hooks behavior by having an entire second implementation of the RTKQ createApi method with the hooks logic mixed in. It would be nice if we could do it as more of a layer on top of the core API. In other words, create a UI-less api instance first, then add the hooks on top of that. The basic public usage would still be the same, just import { createApi } from "@reduxjs/toolkit/query/react", but this would allow generating a UI-less api instance for use on the server side, and then extending it with the React hooks on the client side. It might also simplify other UI integrations as well. (Conceptually, I can almost envision it as a form of injectEndpoints - call methods on the original API instance, return the same API instance but with new capabilities built in and updated TS types?)
  • Infinite queries ala Implementing infinite scroll with RTK Query #1163 and RTKQ "infinite query" potential use cases and concerns #3174
  • A pseudo-normalization approach. If you fetch a list of items, it would be neat to also add those to the store as individual entries as well (so, getTodos() could also save items as getTodo(todoId)). I feel like that could help bridge the gap with some more complex use cases. (Some interesting discussion here: https://twitter.com/bancek/status/1703880605379784832 )

Dependant queries, infinite queries and pseudo-normalization would be the most useful features in my opinion. It would remove a lot of solutions that seem "hacky". For infinite queries, I think it's important to also have refetching. If I have an infinite query with the first 75 items loaded, I should be able to refetch the 75 items with one call. With the pseudo-normalization, it could eliminate a lot of bridging between a slice and RTKQ IMO.

@rcuching
Copy link

rcuching commented Apr 6, 2024

Current painpoint I have is trying to infer the endpointName/querykey for the first parameter of updateQuerydata and I am getting a typescript error.

im trying to use @rtk-query/codegen-openapi to generate the queries and while trying to perform an optimistic endpoint.

So if the generated useQuery is useGetEventTypesQuery() then im trying to grab the name off of it and end up with useGetEventTypesQuery.name or alternatively get it via enhancedApi.endpoints.getEventTypes.name

snippet

    // ts error
    const endpointName: string = enhancedApi.endpoints.getEventTypes.name
    // ts error
    const altEndpointName = useGetEventTypesQuery.name
    // works correctly
    const workingendpointName = 'getEventTypes'

    const tempPatch = dispatch(
      updateQueryData(endpointNamt, originalArgs!, (draft) => {
        draft.result = [...body.body];
      })
    );

ts error

TS2345: Argument of type string is not assignable to parameter of type
QueryKeys<{   getVersion: QueryDefinition<void, BaseQueryFn<string | FetchArgs, unknown, FetchBaseQueryError, {}, FetchBaseQueryMeta>, never, AppVersion, 'api'>;   ... 237 more ...;   getHours: QueryDefinition<...>; }>

Is there a way to work around this? to programmatically get the key instead of having to type the string out, this would allow us to rely on our generated code from the api contract as consts as opposed to strings that someone could overwrite accidentally. well thats my general thoughts and hopes.

if im just flat out not thinking about it in the rtk query way, that could be it too since im newer to using rtk query

@EskiMojo14
Copy link
Collaborator

we could possibly strongly type the .name property without needing to make too many changes (#4332) but i would question how useful that is if to get it you already need to know the endpoint name

you can't get it from a hook though, unless we added an extra property to the function - the fn.name property is built in so i don't think we should touch that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests