Feature/typed query key

[roblabat]

Reopenned from #6127

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
query	⬜️ Ignored (Inspect)	Visit Preview		Oct 23, 2023 8:17am

[codesandbox-ci]

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 3a95c0a:

Sandbox	Source
@tanstack/query-example-react-basic-typescript	Configuration
@tanstack/query-example-solid-basic-typescript	Configuration
@tanstack/query-example-svelte-basic	Configuration
@tanstack/query-example-vue-basic	Configuration

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 41cb821. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

🟥 Failed Commands
nx affected --targets=test:eslint,test:lib,test:types,test:build,test:bundle

---

Sent with 💌 from NxCloud.

[NetanelBasal]

@TkDodo any ETA for this?

[TkDodo]

I don't think there's a need to switch to this, the current implementation works fine. This one is based on an unused generic, which is a bit risky because typescript can choose to "forget" it. Is there any reason you're asking for it?

[NetanelBasal]

Yes. I finished the migration to v5 but the DataTag approach isn't working for me. Btw, the svelte adapter doesn't use it. I'm not sure how it works for you as the signature of the queryOptions queryKey and the useQuery queryKey isn't the same and typescript complains.

[NetanelBasal]

You can see a reproduction here

[TkDodo]

you probably need a conditional type like we have it here:

query/packages/query-core/src/queryClient.ts

Lines 111 to 123 in b671071

	getQueryData<
	TQueryFnData = unknown,
	TaggedQueryKey extends QueryKey = QueryKey,
	TInferredQueryFnData = TaggedQueryKey extends DataTag<
	unknown,
	infer TaggedValue
	>
	? TaggedValue
	: TQueryFnData,
	>(queryKey: TaggedQueryKey): TInferredQueryFnData | undefined
	getQueryData(queryKey: QueryKey) {
	return this.#queryCache.find({ queryKey })?.state.data
	}

[NetanelBasal]

But you don't have it in the useQuery signature, so how does it works for you?

useQuery(queryOptions({ queryKey }))

[TkDodo]

• queryOptions creates a DataTag
• useQuery takes a QueryKey
• you can pass a DataTag where a QueryKey is expected
• we don't have functions that expect a DataTag to be passed, because that would limit us to accept something created by queryOptions. Instead, the functions that could benefit from the type on the DataTag expect a generic that extends QueryKey where we then have the conditional type.

[NetanelBasal]

you can pass a DataTag where a QueryKey is expected

I understand how it works in getQueryData, but how it works with useQuery(queryOptions)? You can see in my basic reproduction that typescript is complains (and it makes sense).

[TkDodo]

I don't understand your basic reproduction because it doesn't do what we're doing in useQuery:

options: { queryKey: DataTag<TQueryKey, TData> }

I guess getOptions should be useQuery ? If so, you're expecting the options that are passed in to to have a queryKey that is of type DataTag, and as I tried to say, that's not what we're doing.

[NetanelBasal]

@TkDodo I make it more clear. Check this please

[TkDodo]

I see. The queryKey returned from our queryOptions functions is an intersection of QueryKey and DataTag. Fixed playground

[NetanelBasal]

But that's not in your code 🤔

https://github.com/TanStack/query/blob/main/packages/react-query/src/queryOptions.ts#L45

[TkDodo]

it is because it's an intersection:

DefinedInitialDataOptions<TQueryFnData, TError, TData, TQueryKey> & {
  queryKey: DataTag<TQueryKey, TData>
}

and DefinedInitialDataOptions defines QueryKey as TQueryKey, so the two get intersected

[NetanelBasal]

That's correct. I don't know what I'm missing. Anyway, thanks, I'll dig into it.

[roblabat]

Fixed playground

I don't think there's a need to switch to this, the current implementation works fine. This one is based on an unused generic, which is a bit risky because typescript can choose to "forget" it. Is there any reason you're asking for it?

@TkDodo I don't understand this point that typescript could choose to "forget" it, what do you mean by that ? Maybe I'm missing something but I don't see any problem on this...

For me there is two reasons to migrate to this approach:

• First the DataTag approach is defining a type which is not compatible with the execution object it refers as our key will never have the [DataTagSymbol] property. Our implementation force typescript to assume the key has this property but I don't think it's a good practice as it breaks typesafety (in our case it could be acceptable as we have no reason to use the [DataTagSympol] property but it's still not a good pratice)
• The second reason which for is the most important is that the other approach could enable an other way of using this by typing the key directly as we could define the options without the queryOptions this way:

const TodoKey = ['todos'] as TypedQueryKey<Todo[]>;

useQuery({ QueryKey: TodoKey, QueryFn: TodoQueryFn });

I personally prefer this definition as I'm sure of my query typing when I define my key but that's more a personal preference.
I still need to use this to have a better point of view on that especially on how it will works with variable dependent queries.

[TkDodo]

First the DataTag approach is defining a type which is not compatible with the execution object it refers as our key will never have the [DataTagSymbol] property. Our implementation force typescript to assume the key has this property but I don't think it's a good practice as it breaks typesafety (in our case it could be acceptable as we have no reason to use the [DataTagSympol] property but it's still not a good pratice)

I don't understand this, can you show an example? We don't expect a key to have the DataTagSymbol, ever. We have two functions that work better if the key has it: getQueryData and setQueryData. But if the key doesn't have it, that's fine, too. Keys can be just Array<string> as well.

could enable an other way of using this by typing the key directly

this is not the intention, because it's not type-safe at all. If we change what the queryFn returns, this will diverge. What good is a typed query key if we don't infer the types from the source of origin? If you want type assertions, you can just as well do them on the getQueryData / setQueryData side.

I don't understand this point that typescript could choose to "forget" it, what do you mean by that ? Maybe I'm missing something but I don't see any problem on this...

I got that info from Andarist (not tagging him because I don't want to drag him into this). Basically, as far as TypeScript is concerned, the generic Data is unused:

export declare interface TypedQueryKey<Data> extends QueryKey {}

so the compiler can choose to eliminate it. So, from TypeScript's perspective, this unused type param doesn't exist since it isn't used by the structure of this type anywhere.

[roblabat]

I don't understand this, can you show an example? We don't expect a key to have the DataTagSymbol, ever. We have two functions that work better if the key has it: getQueryData and setQueryData. But if the key doesn't have it, that's fine, too. Keys can be just Array as well.

You can see this example which show why this approach is a bad practice as it break typescript type safety.

this is not the intention, because it's not type-safe at all. If we change what the queryFn returns, this will diverge. What good is a typed query key if we don't infer the types from the source of origin? If you want type assertions, you can just as well do them on the getQueryData / setQueryData side.

the Idea behind that would be to update useQuery typing to make it check that if we use an already defined TypedQueryKey the typing of the TypedQueryKey is compatible with the returned type of the queryFn to avoid breaking type safety, it's a different approach but both could be compatible letting users choose what is the best for their use case like we currently have both approach of queryOptions and Genericaly typed getQueryData/ setQueryData. You can see Vue Provide/Inject that works this way and remains type safe.

I got that info from Andarist (not tagging him because I don't want to drag him into this). Basically, as far as TypeScript is concerned, the generic Data is unused:
export declare interface TypedQueryKey<Data> extends QueryKey {}
so the compiler can choose to eliminate it. So, from TypeScript's perspective, this unused type param doesn't exist since it isn't used by the structure of this type anywhere.

Maybe I'm missing something on this but that's not how typescript work. We need to make the difference between typescript compiler that make the translation to js and so removes all types during compilation (this includes the TypedQueryKey but also the DataTag which is also a type without any consequence on js compiled code and that's the behaviour we want here). And the ts type checker that handle all types, make typing verifications, inference and is also used for intellisense. On the type checker point of view using the declare keyword tels him to assume that this type exist and use a generic internally he can not ignore it as he doesn't know what is the implementation behind this type (in our case it's a simple array but it be something more complicated in js like object prototyped from the Array object) the declared keyword is used to define typings for external libs that are not written in typescript or to extend typings definition without changing code implementation that's why typescript doesn't complain about our unused generic here (it would be different without the declare keyword).
In conclusion yes the compiler will ignore the TypedQueryKey but it is intentional and that's exactly the for the DataTag but no the typed checker will not ignore it, this is already used in production by big libs like VueJs so there is no reason to think differently in our use case.

[TkDodo]

regarding:

const test = todosOptions.queryKey[dataTagSymbol];

the symbol is for internal use and we shouldn't expose it to consumers.

---

I wasn't talking about the compiler in terms of "stripping types" for js emit. Of course it strips types here. I was actually referring to the type checker. The Generic on the TypedQueryKey type is unused so the type checker can choose to forget it. It doesn't do it at the moment, and it works, and maybe it will never change, but I don't see why this is better than the current approach. It looks like you want it to do some things that weren't intended, like:

const TodoKey = ['todos'] as TypedQueryKey<Todo[]>;

useQuery({ QueryKey: TodoKey, QueryFn: TodoQueryFn });

no, that's not good. The only way to create a typed query key should be via queryOptions, and having a private symbol assures this. In that example, if the QueryFn changes and returns NotTodo[], you'll get out of sync with this type assertion on TodoKey. If you have queryOptions and you change the queryFn there, the type inferred on the key will change with it, and all the occurrences of queryClient.getQueryData(TodoKey) will change, too.

[roblabat]

the symbol is for internal use and we shouldn't expose it to consumers.

Yes if we keep this approche we should at least remove this symbol from exports.
But I'm not sure this symbol will not limit us in more advanced use cases (mainly on keys using variables but I need more investigations on this)

I wasn't talking about the compiler in terms of "stripping types" for js emit. Of course it strips types here. I was actually referring to the type checker. The Generic on the TypedQueryKey type is unused so the type checker can choose to forget it.

This would be a huge breaking change if typescript choosed ignore that kind of generic types, as the declare keyword is used to tell typescript that you are typing an already existing code the code checker need to keep all given information as it can represent some typing informations that are not represented directly in the object (like our typing linked to the key). Other big libs rely on this mechanism so I don't think this eventuality should be considered as this kind of breaking changes can't be anticipated...

It looks like you want it to do some things that weren't intended, like:const TodoKey = ['todos'] as TypedQueryKey<Todo[]>; useQuery({ QueryKey: TodoKey, QueryFn: TodoQueryFn });
no, that's not good. The only way to create a typed query key should be via queryOptions, and having a private symbol assures this. In that example, if the QueryFn changes and returns NotTodo[], you'll get out of sync with this type assertion on TodoKey. If you have queryOptions and you change the queryFn there, the type inferred on the key will change with it, and all the occurrences of queryClient.getQueryData(TodoKey) will change, too.

That's exactly what I want to do but it doesn't break type safety. Yes if the queryFn returns change it will not be propagated to occurrences of queryClient.getQueryData(TodoKey) but it will be thrown directly on the queryOptions definition as I want to check this directly in the useQuery and queryOption functions parameters. I made a quick example of this here see the errors on test3 and test4 queryFn returns that doesn't match the expected return type extracted from the queryKey. I'm not saying this way of defining the queryOptions is better or worst but in some use case it could make sense. For example in my team multiple developers and screens relies on the same queries so we bundled the implementations of or useQuery function in custom hooks that are called in components. Using this new patern would make any change on the queryFn to throw directly in our custom hook, as it's return type is not any more the expected one, instead of throwing in all components using it, this behavior would be easier to troubleshoot for the developer that made a change to the queryFn but is not responsible for all its implementations in components. I think it would be a good think to make the lib compatible with both approaches if approaches are both typesafe. What to you think of this ?

[roblabat]

@TkDodo any updates on this ?