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

Update method query observer base result #3636

Closed

Conversation

sahilmob
Copy link

@sahilmob sahilmob commented May 21, 2022

The aim of this pr is to expose a convenience update function that has the query key in its closure scope and internally calls queryClient.setQueryData(key, updater).

In many cases, there is a need to optimistically update query data inside the same component that calls useQuery, so instead of using queryClient.setQueryData(key, updater), update comes preloaded with the right query key in its closure scope and internally calls queryClient.setQueryData(key, updater).

So it allows for the following

function MyComponent {
 const { update } = useQuery(key, ...);

 const issueApiRequest = (newValue) => {
   update(newValue);
   fetch(...);
 }
}

instead of

function MyComponent {
 const { update } = useQuery(key, ...);
 const queryClient = useQueryClient();

 const issueApiRequest = (newValue) => {
   queryClient.setQueryData(key, newValue);
   fetch(...);
 }
}

@vercel
Copy link

vercel bot commented May 21, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-query ✅ Ready (Inspect) Visit Preview May 21, 2022 at 2:34PM (UTC)

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 06d4128:

Sandbox Source
tannerlinsley/react-query: basic Configuration
tannerlinsley/react-query: basic-typescript Configuration

@TkDodo
Copy link
Collaborator

TkDodo commented May 23, 2022

I'm not sure there is a real use-case for this function. Usually, you'd do what you are trying to do in the onMutate function of an optimistic mutation. And there, you likely need the useQueryClient anyways

@sahilmob
Copy link
Author

Of course the same result could be achieved using useQueryClient. This is just a convenience function so that it already has the query key inside it closure scope. This is particularly useful when you want to update a query inside a component that already has access to that query. So basically you get to avid the extra import of useQueryClient and the extra hook call.

@u3u
Copy link

u3u commented May 28, 2022

https://swr.vercel.app/docs/mutation#optimistic-updates
I've been looking for something like swr mutate(data, false), where I want to be able to update locally directly with the data returned by the remote interface when I do an update, rather than re-querying it. useQuery returns a refetch method that doesn't provide an option for that either, but only through useQueryClient() and queryClient.setQueryData() to update the local data, which is a pain, so I need to wrap useQuery to do it myself.

@sahilmob
Copy link
Author

@u3u You are right. Importing useQueryClient, calling it, and then calling queryClient.setQueryData() is a kind of pain, especially maintaining the query key for both the query itself and the queryClient.setQueryData() call. You might change the query key for any reason and forget to change it in queryClient.setQueryData(). This PR helps in avoiding maintaining the query key across two function calls.

@TkDodo
Copy link
Collaborator

TkDodo commented May 29, 2022

You might change the query key for any reason and forget to change it in queryClient.setQueryData()

You can say the same thing about all functions on the query client, like queryClient.invalidateQueries, too. I've always advocated for query key factories for that reason.

Adding new functionality that can already be achieved in different ways is always a trade-off. Some people already complain about bundle size, so it's very hard to make everybody happy. Adding a feature should imo yield a distinct functionality for a common case. The biggest gain that I can see currently by this proposal is the type-safety, because queryClient.setQueryData needs a generic param to get type-safety, while the one returned from useQuery can be bound to TQueryFnData correctly. You haven't mentioned this advantage at all, so my guess is you're not using TypeScript? The PR would also need some more love for types, because updater: any is not going to get merged as-is 😅 .

I've been looking for something like swr mutate(data, false), where I want to be able to update locally directly with the data returned by the remote interface when I do an update, rather than re-querying it.

It's documented here on how to do that with the current apis. Yes, it involves useQueryClient and then queryClient.setQueryData. However, I'm not so sure if setData returned from useQuery would help in this case. Often, mutations happen in different components from where the query happens. We also advocate for custom hooks, so if you have a useUpdatePost hook, you'd need to pass the function as a parameter from a custom usePost(id) hook to that hook. I'd argue that's even more complexity than just calling useQueryClient.


I will need to think about the tradeoffs a bit more. In the meantime, you can already build that quite easily in user-land:

const useMyQuery = (key, fn, options) => {
  const queryClient = useQueryClient()
  return {
    ...useQuery(key, fn, options),
    update: (dataOrUpdater) => queryClient.setQueryData(key, dataOrUpdater)
  }
)

@sahilmob
Copy link
Author

Yeah, currently, I'm not using typescript. However, if, by any chance, bounding TQueryFnData is the only holdup, I can work on it.

@TkDodo
Copy link
Collaborator

TkDodo commented Jun 5, 2022

@sahilmob the main point would actually be:

Often, mutations happen in different components from where the query happens. We also advocate for custom hooks, so if you have a useUpdatePost hook, you'd need to pass the function as a parameter from a custom usePost(id) hook to that hook. I'd argue that's even more complexity than just calling useQueryClient.

Can you show a concrete example where the function would be useful?

@sahilmob
Copy link
Author

I would argue that often, especially in tabular data, mutations happen in the same component where the query happened, for example, this simple todo list, toggling a todo status and the query to get the todos both happened in the same component. This is also true if you're using a components library, i.e, Material UI, which allows you to create a fully functional table with no more than 25 lines of code, including the logic for rendering a single row, which might contain a toggle for activating/deactivating a row for example. A transfer list is also a good example.

@TkDodo
Copy link
Collaborator

TkDodo commented Jun 13, 2022

Thanks for the examples - let me try to explain why I don't think that they are representative:

  • the simple todo list

if checking off the todo should perform an optimistic update, this example leaves out a bunch of code. The toggling itself would happen within the onMutate callback of useMutation. You would likely need the queryClient anyways to do queryClient.invalidateQueries in onSettled to get the correct server state.
Even if you write the response from the mutation directly to the query cache without refetching, it would still be the better idea to extract this into a custom hook that's separate from the query. If you do that, you won't have access to the function returned from useQuery.

  • transfer list

Let's assume the mutation should happen once you click the buttons in the middle that transfer entries from left to right. Selecting entries on the left is pure client state up to the point where you trigger the mutation. You should definitely not write to the queryCache when the user checks one of those boxes, because that selection would be overwritten with any background update. Suppose the following scenario:

  • You select 5 entries
  • An email comes in, you switch to your gmail tab, answer it
  • You go back to your transfer list. The query is now stale, so the window focus will trigger a refetch
  • Your selection will be re-set to the current server state, so you have lost your selection.

Things like this are the reason why I think exposing this function is not a good idea - it will encourage people to write directly to the query cache on the client side, which you likely shouldn't be doing. The query cache is a client side "mirror" of the state the server currently holds. The only situation where it makes sense to write to it directly is for optimistic updates, where you are telling the cache that this is the state we expect the server to be in anyways in a couple of seconds, or to update from mutation responses, where the server tells you that this is the current server state, and you want to set it directly instead of refetching.

@sahilmob
Copy link
Author

Thanks for your detailed answer. However, If you ask me, I would not do any assumptions around how people would wan't to use any api exposed to them. because they could update the query cache in a way that doesn't reflect the server state with userQueryClient, Its just a matter of convenience. Anyway, I just wanted to explain my use case and now its your call.

@TkDodo TkDodo closed this Jun 20, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants