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

Return query key from useQuery #467

Open
anuraaga opened this issue Oct 23, 2024 · 4 comments
Open

Return query key from useQuery #467

anuraaga opened this issue Oct 23, 2024 · 4 comments

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Oct 23, 2024

useQuery is the primary entrypoint for fetching data and manages creating a query key and setting the query function as per the README. I was wondering if it would make sense to have it return the query key it created? It feels a bit tedious to have to create it again in mutations.

const queryClient = useQueryClient();

const { data: getStuffRes, queryKey: getStuffKey, isPending } = useQuery(getStuff, {
    stuffId,
  });

// No need to call createConnectQueryKey(getStuff, { stuffId }), which repeats args to useQuery

const doAddStuff = useMutation(addStuff, {
  onSuccess: (resp) => {
    queryClient.setQueryData(getStuffKey, (prev) => {
      return {
        ...prev,
        stuff: [...prev.stuff, resp.newStuff],
     };
    }
  }
});
@paul-sachs
Copy link
Collaborator

paul-sachs commented Oct 24, 2024

Hi @anuraaga, this is an interesting proposal. We could of course do it ourselves, although this feels more like something that would benefit all of @tanstack/query. In fact, this item was already brought up here. Part of me wants to hew close to whatever tanstack query does but I can see the value here.

I think the only argument I really see against returning the queryKey is that that key is now disjoint from the associated data stored in the cache. Which means that it will allow you to do something like:

queryClient.setQueryData(getStuffKey, { somethingTotallyWrong: true });

Which will cause everything to break.

We have some potential work coming after the v2 release that eases this slightly with a custom queryClient that would instead allow you to do something like:

queryClient.setConnectQueryData(getStuff, { stuffId }, (prev) => {
      return {
        ...prev,
        stuff: [...prev.stuff, resp.newStuff],
     };
    });

This method enforces type safety and makes sure that the new data incoming matches the cached data types. It still requires you building the same input, but I personally think that's pretty minor compared to ensuring static type safety.

@anuraaga
Copy link
Contributor Author

Thanks for the consideration @paul-sachs. TBH I have only every used react query with connect, so I'm not sure on idiomatic vanilla usage but IIUC there it didn't make much sense since the user creates the key, and it's trivial to just reference that without returning it. Here, since connect-query is creating it, it could make sense to return what it created.

The final blog post linked in the discussion was interesting

https://tkdodo.eu/blog/the-query-options-api#datatag

It seems query already has the concept of associating the return type with the key! If the returned key has the datatag with the response type, it seems like the other methods like setQueryData should be type-safe even without a custom QueryClient. Perhaps this provides even more motivation to return the key?

@anuraaga
Copy link
Contributor Author

anuraaga commented Oct 25, 2024

BTW, I prototyped the idea here

curioswitch/aiceo@ecfc700

The key point is calling queryOptions from tanstack-query which adds the response type to the query key. Thanks to that, setQueryData below is type-safe and I could remove the protobuf-safe updater, at the expense of not easily being able to use a partial message but that's best solved by updating to connect/protobuf-es v2.

This was actually my first experience with connect-query's createQueryOptions so I then tried going a bit further to match the query factories pattern in the linked blog and it seems to go pretty well.

curioswitch/aiceo@9b080fd

So I guess I don't see a big reason to return query key anymore, while it could be convenient for useQuery from this library, it feels idiomatic to instead do createQueryOptions and stick to useQuery from react-query. Some possible suggestions then become

  • Add useQueryOptions which implicitly gets the transport from context. It's not needed if going with the query factory pattern but could be an alternative to the current connect-query's useQuery that additionally allows keeping track of the query key
  • Call queryOptions from above APIs to have the typesafe query key. Not sure if it's practical, an initial attempt by me makes it seem quite difficult to have compatible types and may require a TypeScript guru. No problems when calling within my own code though
  • Add createMutationOptions (and if going with above, useMutationOptions) to allow mutations to be symmetric to usage of the query options API, specifically a way to let connect-query create the mutationFn while using tanstack-query's native useMutation.

@paul-sachs
Copy link
Collaborator

Interesting note on the dataTagSymbol, I'll need to experiment to see if I can get that working consistently with our query key types.

It doesn't entirely eliminate the usefulness of a custom query client, but it does make me think about how we can reduce our surface area. Having it for fetch/prefetch are likely still enough to have it exist but we'll see how investigations go.

You can also use createConnectQueryKey directly, without everything else created by createQueryOptions but it does depend on the context.

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

2 participants