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

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

Open markerikson opened 1 year ago

markerikson commented 1 year ago

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 https://github.com/reduxjs/redux-toolkit/discussions/3174 and https://github.com/reduxjs/redux-toolkit/discussions/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:

Dovakeidy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@nhayfield can you give a couple further details?

nhayfield commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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?

https://github.com/reduxjs/redux-toolkit/discussions/3506 https://github.com/reduxjs/redux-toolkit/pull/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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

rwilliams3088 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

markerikson commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

codingedgar commented 1 year ago

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 commented 1 year ago

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

https://github.com/reduxjs/redux-toolkit/assets/10248395/af16a2a9-0dce-4934-8257-5bf06f7e3686

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 commented 1 year ago
  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 commented 1 year ago

@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?

bever1337 commented 1 year ago
* 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?

I use both of these features today! I try not to rely on either, IMO requires a lot of comfort with the library so I try not to inflict it on others, but they come up.

dependent queries with a queryFn -

IME this works great and as-expected. One pain point is when both queryFn and onCacheEntryAdded handlers subscribe to the same endpoint. IME this approach is a little noisy, user error may lead to leaky subscriptions, and cyclic-dependencies need to be considered if the RTKQ codebase have complex dependencies over many modules.

invalidating tags with queries -

During a long-running cache entry lifecycle, it can be useful to 'kick' failed queries when it's obvious they might succeed if retried, ex: websocket came back up, data is now available. An alternative is to put a polling interval on the query. The downside of the polling interval is unnecessary waiting.

In the few times I've manually invalidated tags outside of a mutation, it's been an attempt to alter the cache state, i.e. it failed but now I am positive the query would succeed. If the cache state were good, then the cache entry could be updated without going through invalidation. In some cases, I may even have a valid next cache value derived in onCacheEntryAdded, but calling updateCachedData is a No-op.

IIUC upsertQueryData is the new alternative when an onCacheEntryAdded handler wants to manually correct query state, but I haven't tested enough to understand what it affects. If it works how I think it works, I believe this is better for the use-case of a cache entry manually correcting cache state, but there are use-cases for re-trying the entire query via tag invalidation too.

I really liked the idea of an invalidateCachedData helper passed to onCacheEntryAdded. upsertQueryData feels like a footgun. I am unsure if it should accept tags for manual invalidation, or if it should only invalidate the cache key.

graphql -

I worry this is feature bloat. Instead of prioritizing just graphql, I'd be curious to hear what's missing from the base library that makes it difficult to write reusable queryFn/onCacheEntryAdded handlers with a graphql backend. Is there a reason why userland isn't able to tackle this today? (Besides possibly interest and critical mass?)

useQuery return value -

I loved the idea of useQuery returning a tuple! This is a hugely breaking change and will be very tedious for me, but I'll really appreciate it someday.

bever1337 commented 1 year ago

There's two threads I am not following:

  1. Besides the clunkiness of onCacheEntryAdded promises, I do not follow the ongoing conversation re:cancellation of queries. RTKQ provides a signal to queryFn and uses the same signal for cancellation with fetch base query. What's missing? If it's not a pain, can someone clarify? Thanks!
  2. From the original post, "The cache lifecycle methods do not run when there's a cache hit." Is this specific to hydration? How does a store have a cache entry without ever calling onCacheEntryAdded?
dwjohnston commented 1 year ago

I would like an imperative way to use the lazy queries outside of a React context (ie. within other queries).

For example, say I have a slice like:

const todoSlice = createApi({
    endpoints: (builder) => {
         addTodoToUser: queryFn: async (args) => {
               const {userId, todoText} = args; 

               // Now I need get the full user object for some reason
               // What I'd like to do is               
               const user =await usersSlice.endpoints.getUserById.imperativeQuery({userId}); 

               //Where it'll basically have the same behaviour as being like: 
               const [getUser] = usersSlice.endpoints.getUserById.useLazyQuery(); 
               const user = await getUser({userId});
               // Of course, I can't do this, because I'd get a rules of hooks warning.  

         }); 
    });
});

I know that I could use the initiate and select methods, but that's kind of gross, (what am I going to do, poll until it's resolved?), I want to use this using async behaviour.

Here's my utility function that does this:

export async function imperativeLazyQuery(endpoint, api, args) {
  api.dispatch(endpoint.initiate(args));
  return new Promise((res, rej) => {
    const interval = setInterval(() => {
      let value = endpoint.select(args)(api.getState());
      if (value.isSuccess || value.isError) {
        clearInterval(interval);
        res(value);
      }
    }, 100);
  });
}

Having a hard to writing typings for it though.

Dovakeidy commented 1 year ago

@dwjohnston You can use the unwrap method to get the result with async/await.

const response = await api.dispatch(endpoints.initiate(args)).unwrap();

isqua commented 1 year ago

I would like to have the following features:

  1. Some simple solution for ā€œinfinity scrollā€ problem, as has already been mentioned by many.
  2. Easier updating a query data with its friend-ish mutation data.
  3. Declare some placeholder data for an endpoint.

@eric-crowell put it wonderfully:

Honestly, in my mind, RTK Query is a tool to cleverly handle how to dispatch actions on a redux store when fetching data.

Without placeholders I have to put some initial/placeholded data into selectors. Itā€™s not a big deal, just a small inconvenience. But also, I hope itā€™s not that hard to implement.

What is much more complicated for me, is to update data for cached queries from mutations. Itā€™s a common pattern when POST/PUT/PATCH endpoit returns new data, so what I need is just to put the data into the store.

E.g. I have such endpoints for managing current user profile:

const profileApi = api.injectEndpoints({
    endpoints: (build) => ({
        getProfile: build.query<Profile, undefined>({ query: () => "/profile" }),
        updateProfile: build.mutation<Profile, Partial<Profile>>({
            query: (profile) => ({
                url: "/profile",
                method: "PATCH",
                body: profile,
            }),
        }),
    }),
});

So after successful profile update Iā€™d like to put the API response as a cached value of getProfile endpoint, I do not actually need to refetch the data. And that part is too complicated for me, So I decided just to write a simple slice with profile data and add a pair of matcher-based reducer to it.

It would be wonderful to have simple onSuccess handler for mutations that allows to make some changes with a state:

const profileApi = api.injectEndpoints({
    endpoints: (build) => ({
        getProfile: build.query<Profile, undefined>({
            query: () => `/profile`,
            placeholderData: initialState,
        }),
        updateProfile: build.mutation<Profile, Partial<Profile>>({
            query: (profile) => ({
                url: `/profile`,
                method: "PATCH",
                body: profile,
            }),
            onSuccess: (result, _args, api) => api.util.updateQueryData(
                "getProfile", undefined,
                (state: Profile) => Object.assign(state, result.data)
            )
        }),
    }),
});

In order not to flood this issue with my code, I put a more detailed example of what I am coding and how I would like to see it in this gist.

markerikson commented 1 year ago

btw, quick update: we're still very actively interested in improving RTKQ's API, and still soliciting suggestions here in this thread.

Per https://github.com/reduxjs/redux-toolkit/issues/958#issuecomment-1742188595 , we're not going to make any meaningful changes to RTKQ's API for RTK 2.0, so that we can get this long-running effort wrapped up and shipped in the near-ish future.

We will then start focusing on RTKQ changes after that, and we're willing to ship another major as 3.0 in the near future if any of the changes would be breaking.

chriskuech commented 1 year ago

I only use RTKQ via the openapi codegen, so my perspective may be confusing, but there they give you hook args matching your API args. If these args change, a new request is fired to keep the hook in desired state; however, the API calls still continue even though (a) they are GETs and should be idempotent, and (b) no one will ever see their responses.

This is especially problematic because the browser limits the number of concurrent calls to a domain, requiring messy hacks.

msolano00 commented 1 year ago

One nice to have feature for me would be the ability to (easily) track a global loading/fetching flag.

There are some use cases where I can see applications wanting to block all user interaction with things like filters or so until all data fetching is fulfilled/failed.

markerikson commented 1 year ago

@msolano00 : fwiw the simplest answer to that atm is to write a selector that loops over all the cache entries in state:

const selectAnythingLoading = (state: RootState) => {
  const { queries } = state[api.reducerPath];
  const anythingLoading = Object.values(queries).some(entry => entry.status == "pending"
  return anythingLoading;
}

(off the top of my head, but probably pretty close.) You can also do things like checking to see if the serialized query cache key matches some endpoint names, etc.

(Edited by Lenz)

Noor0 commented 11 months ago

Thanks for the thread.

I think the ability to query pending/fulfilled/rejected mutations for one or multiple endpoints will be very helpful.

api.endpoints.someEndpoint.getRejectedMutations()
api.endpoints.someEndpoint.getFulfilledMutations()
api.endpoints.someEndpoint.getRunningMutations()
Noor0 commented 11 months ago

Another great addition will be the ability to tag mutation with multiple keys (fixedCacheKey: ["some", "mutation", "key"]) like in react-query and the ability to query all running mutations based on some predicate function or combination of cache keys (rtkQuery.getAllMutations(args: { status?: "pending" | "fulfilled" | "rejected", cacheKey?: []; predicate?: (...args: any[]) => boolean }))

rjgotten commented 11 months ago

@dwjohnston You can use the unwrap method to get the result with async/await.

const response = await api.dispatch(endpoints.initiate(args)).unwrap();

FWIW I'm currently on RTK version 1.9.5 and there at least this doesn't actually work. Query results consistently come back just as undefined.

Presumably unwrap just resolves the thunk promise and pulls out whatever is in data. Which may be a cached result; or may be undefined if the actually query is still running.

You'd still need to actually wait until the request finishes, in case isLoading is true. Follow-up assumption would be that getRunningQueryThunk could be used for that. getRunningQueryThunk is a PitA to use though and TypeScript typing somehow goes down the black hole with it...

EskiMojo14 commented 11 months ago

@rjgotten the promise from initiate shouldn't resolve until the query has already finished - any chance you could put together a codesandbox or replay of what you're seeing?

rjgotten commented 11 months ago

@rjgotten the promise from initiate shouldn't resolve until the query has already finished - any chance you could put together a codesandbox or replay of what you're seeing?

I figured it out in the mean time. The query had a subscribe: false added to its options; the idea being that we didn't want a subscription.

But apparently this causes the query to execute and then throw out its results (because nothing is subscribed?) before they can even be reported back out as the result of the initiate thunk. (I double-checked and the actual return value from initiate has an isUninitialized:true in this case.)

Not sure if that should be considered a bug or not. But it certainly is quirky.

EskiMojo14 commented 11 months ago

ah, yeah - the behaviour around subscribe: false is non-ideal. That's something we're aiming to fix in v2.0 - take a look at https://github.com/reduxjs/redux-toolkit/pull/3709.

rjgotten commented 11 months ago

Awesome. That is excellent news then. šŸ˜„

MustafaHaddara commented 11 months ago

I find the queryFn interface a little awkward. Currently, I'd have to write this:

queryFn: async (args, api, opts, baseQuery) => {
  let result = await baseQuery(args);
  // etc...
},

but since baseQuery will automatically pass through api and opts, I think that the majority of use cases where I want the baseQuery param, I will never need the api or opts params, and then I get lint warnings for unused params.

I think I'd prefer being able to do

queryFn: async (args, { baseQuery }) => {
  let result = await baseQuery(args);
  // etc...
},

In addition, if you rewind and ask "why are you using queryFn anyways?", in my case the answer is that the transformResponse and transformErrorResponse aren't good enough for me. I have to interact with a slightly misbehaving API endpoint that returns a 404 as a normal occurrence. Ideally, I want clients of the hook to not care, so my queryFn above looks a bit like this:

queryFn: async (args, { baseQuery }) => {
  let result = await baseQuery(args);
  if (result.status === 403) {
    return { data: [] };
  } 
  if (!result.ok) {
    return { error: 'Something went wrong' };
  }
  return { data: result.json() };
},

If there was a lower level transformResponse that got called on all responses, then I wouldn't even need a queryFn at all.


An alternative would be to make it easier to wrap the generated query, like so:

export const useGetMyData = (args: MyParams | SkipToken, options: UseQueryOptions) => {
  let { data, error, ...rest } = useGetRealApiQuery(args, options); // as generated from createApi()

  if (error.response.status == 404) {
    data = [];
    error = undefined;
  }

  return { data, error, ...rest };
};

However, the Typescript compiler wasn't a fan of this. I couldn't import UseQueryOptions (despite that being the name of the type in the docs!) and the return type was also unclear. I think there's a TypedUseQueryHookResult helper but that's defined to accepts 3-4 type args, and all I have is the type of the data I am expecting it to return...

Dovakeidy commented 11 months ago

@MustafaHaddara

For the first point, one way of accessing baseQuery without going through api and opts would be to use the spread (although I agree that it's better to have it as an object) :

queryFn: async (args, ...props) => {
  let result = await props.baseQuery(args);
  // etc...
},

For the second point, I think you could use baseQuery customization if you need this logic on all your hooks:

An example of customization with automatic re-authorization by extending fetchBaseQuery

EskiMojo14 commented 11 months ago

@Dovakeidy that isn't how rest arguments work when they're positional, it just gives you an array.

queryFn: async (args, ...rest) => {
  const baseQuery = rest[2]
  let result = await baseQuery(args);
  // etc...
},
Dovakeidy commented 11 months ago

@Dovakeidy that isn't how rest arguments work when they're positional, it just gives you an array.

queryFn: async (args, ...rest) => {
  const baseQuery = rest[2]
  let result = await baseQuery(args);
  // etc...
},

My bad, typed a little too fast ahah

MustafaHaddara commented 11 months ago

@Dovakeidy

For the second point, I think you could use baseQuery customization if you need this logic on all your hooks:

An example of customization with automatic re-authorization by extending fetchBaseQuery

The problem is that I need this logic only on one endpoint.