lukemorales / query-key-factory

A library for creating typesafe standardized query keys, useful for cache management in @tanstack/query
https://www.npmjs.com/package/@lukemorales/query-key-factory
MIT License
1.18k stars 32 forks source link

Type QueryClient setQueryData #38

Closed Lesik closed 1 year ago

Lesik commented 1 year ago

(Copied from Discussions because it seems to have gone unnoticed there.)

Hi,

first off, thanks for the great package! I've been wanting to have something like this ever since I started using React Query over a year ago.

Open here for some personal backstory why I love this package My main problem with React Query is that for queries there should be a single source of truth that links query key, query function, query parameter types and query return types together. But the `useQuery` hook makes you to define these relationships anew over and over again every time you call the hook, which wastes time, makes it easy to introduce an inconsistency, and makes it hard to refactor. So to solve this for the projects at work I had basically written my own "wrapper" around React Query which is very similar to what this package is doing, but since working with complex TypeScript type definitions can be quite daunting I didn't get as far as you guys. Which is why I was happy to finally discover this package! All this time I couldn't believe nobody else had the same problem with React Query as I did and created a package for it. I'm not sure if you're familiar, but Redux Toolkit has a library similar to React Query called RTK Query, and I much prefer their structure: You first define your API [using the `createApi` function](https://redux-toolkit.js.org/rtk-query/overview#create-an-api-slice), where you define all the keys, functions, and data types. This was my main inspiration, but RTK Query goes a bit "too far" by (1) being centered around URLs and endpoints (not flexible for other types of APIs) and (2) requiring Redux which isn't a dependency necessary for every project. So React Query + a helper library is my preferred solution. ---

The feature I propose is to enable typing of the QueryClient's setQueryData function. Through the queryFn field defined on the queries in the createQueryKeyStore function, we already give the type information of the data that this query will hold. Based on that we could type the setQueryFunction to accept only this type.

The advantage is big: Currently, you can set the query data to any data you like, and then the type promise that useQuery() gives your components no longer holds true. It is up to the developer to keep track which query keys should contain what data types, and pass the right data every time. There are no type checks.

Example:

import { createQueryKeyStore } from "@lukemorales/query-key-factory";
import { useQueries, useQuery, useQueryClient } from "@tanstack/react-query";

type User = {
    name: string;
};

const api = {
    getUser: (userId: string): User => ({
        name: "John",
    }),
};

export const queries = createQueryKeyStore({
  users: {
    all: null,
    detail: (userId: string) => ({
      queryKey: [userId],
      queryFn: () => api.getUser(userId),
    }),
  },
});

export const App = () => {
    const query = useQuery(queries.users.detail('14'));
    if (query.isSuccess) {
      // `query.data` is of type `User`, so it knows what type this query holds
      // we can safely access `query.data.name` and it will work
    }

    const queryClient = useQueryClient();
    queryClient.setQueryData(queries.users.detail('14').queryKey, () => ({
        // we can just pass any data here, including an object that contains no `name`
        // which will break the code above trying to access `name`
        email: "john.doe@example.com",
    }));
}

And other functions such as getQueryData should also be type-safe.

Use cases include Update from Mutation Responses and Optimistic Updates.

As for the technical realization, either we could override React Query's type for QueryClient to be type safe using Module Augmentation, or by exporting our own TypedQueryClient and useTypedQueryClient() hooks are just wrappers around the original but with the proper type.

lukemorales commented 1 year ago

Hi @Lesik, so sorry about not noticing you topic in Discussions. I'm missing some notifications to be aware that people are actually using it for the package, still getting used to a OSS package that got that much attention 🙏🏻

I think we can expose a simpler solution from the library that does not mess up with the original tanstack/query types: in the same way that we have a inferQueryKey type, we can also have a inferQueryData type helper that can be used as:

const queryClient = useQueryClient();
queryClient.setQueryData<inferQueryData<typeof queries.users.detail>>(
  queries.users.detail('14').queryKey,
  cachedData => {...}
  // ^? cached data has correct type inferred from the return of `queries.users.detail.queryFn`
)

Should be pretty simple to implement, do you wanna contribute with the type helper? That'd be great

Lesik commented 1 year ago

I would definitely like to try to contribute, although since this would be my first larger TypeScript project it's a bit daunting. Could I contact you via the email in your profile if I have questions or get stuck? Thanks!

I like your proposal with the inferQueryData helper. One concern I have though, is that in a team with less experienced developers, it's very likely that someone will forget to explicitly pass the generic type to setQueryData, which will generate no warning whatsoever. It would be nice if I could (in my codebase) set the setQueryData function to always have the generic type set to inferQueryData (I think through Module Augmentation that should be possible).

However in that case the problem arises that since inferQueryData operates on only one specific query (such as queries.users.detail) and not a whole key store (such as queries), you couldn't set the generic type for the entire project, since it would differ from query to query. It would be nicer if you could inferQueryData on the entire key store, and then let it use the first parameter (the query key) to infer the type of the second parameter (the query data).

Example:

const queries = createQueryKeyStore({
  users: {
    detail: (userId: string) => ({
      queryKey: [userId],
      queryFn: () => {} as User, // dummy response
    }),
    settings: (userId: string) => ({
      queryKey: [userId],
      queryFn: () => {} as UserSettings, // dummy response
    }),
  },
});

queryClient.setQueryData<inferQueryData<typeof queries>>(
  queries.users.detail('14').queryKey,
  (cachedData) => {...}, // cachedData is inferred to be `User` based on the query key in the line above
);

// notice the same generic type, `typeof queries`, being passed to both `setQueryData()` calls
queryClient.setQueryData<inferQueryData<typeof queries>>(
  queries.users.settings('14').queryKey,
  (cachedData) => {...}, // cachedData is inferred to be `UserSettings`
);

Another nitpick that's a bit unpleasant is that in your code example, you have to repeat the query twice (once in inferQueryData<typeof queries.users.detail> and then right below in queries.users.detail('14').queryKey).

Do you agree with my critique points, and if yes, do you think the approach is feasible to accomplish in TypeScript?

lukemorales commented 1 year ago

One concern I have though, is that in a team with less experienced developers, it's very likely that someone will forget to explicitly pass the generic type to setQueryData, which will generate no warning whatsoever.

Not sure how strong of an argument this is; not passing the generic will only prevent you from having type inference. Also, in a team with less experienced developers, there's usually at least one experienced developer helping review code. Still, not using the generic it's not the worst thing in the world.

However in that case the problem arises that since inferQueryData operates on only one specific query (such as queries.users.detail) and not a whole key store (such as queries), you couldn't set the generic type for the entire project, since it would differ from query to query.

I agree that the DX would be better if what you're proposing was achievable, but it revolves more around how to tanstack query designs these APIs rather than how we can "fix" it. It's out of our scope to augment the tanstack query API in anyway.

Another nitpick that's a bit unpleasant is that in your code example, you have to repeat the query twice (once in inferQueryData and then right below in queries.users.detail('14').queryKey).

It's not the most DRY implementation indeed, but I'm not sure if there's a different way of handling that without augmenting the setQueryData API

bisak commented 8 months ago

Hey, what did you guys end up doing in this regard? What's the current preferred way to make setQueryData really type safe?