quickest-silver

FullSearchSchema - why do we need it at all?

While my PR #863 (https://github.com/TanStack/router/pull/863) fixed the issue #859 (https://github.com/TanStack/router/issues/859), I hit the following problems in my real-world app that uses a few search schemes which I guess are caused by the usage of recursive types: 1. TS compilation is now slooooow 2. I get the following TS error depending on how my route tree looks (I have not yet figured out the pattern): Type instantiation is excessively deep and possibly infinite. This made me wonder: Why do we need the FullSearchSchema type at all? FullSearchSchema is the intersection of all routes' search params. It's mainly used in SearchParamOptions. Do I understand this correctly? 1. If from is not specified, then the default value / is used 2. If from equals /, TFromSearchEnsured will be FullSearchSchema But shouldn't the search params in this case be the search params of the root route instead?

46 Replies

quickest-silverOP•2y ago

or put differently, if from is not specified, shouldn't TFromSearch be the union of all possible fullSearchSchema types?

type TFromSearch = RouteByPath<TRouteTree, string>['types']['fullSearchSchema'];

type TFromSearch = RouteByPath<TRouteTree, string>['types']['fullSearchSchema'];

quickest-silverOP•2y ago

I created this PR: https://github.com/TanStack/router/pull/867

GitHub

fix: do not use intersection for search params by schiller-manuel ·...

xenial-black•2y ago

I think this is the same issue I'm encountering, I thought my machine has gotten slow lately, which just < 50 routes. inference to the route has just gotten very slow

genetic-orange•2y ago

We need to revert this then And figure out a better way The default needs to be good, and most people won’t provide a from. So having the root search parameters, plus any Partial search from from all other routes is a good default

quickest-silverOP•2y ago

but why not this? if from is not specified (or is a runtime string ), the search params can be any of the union of all fullSearchParam

genetic-orange•2y ago

I’ll play with that pr asap

xenial-black•2y ago

this is an example on my end @Manuel Schiller https://stackblitz.com/edit/github-wcfnxr?file=src%2Fquery-client.ts,src%2Fmain.tsx

StackBlitz

Large Codebase? - StackBlitz

A forked version of kitchen sink + react query

xenial-black•2y ago

Is this the same issue were encountering? For some reason as well tsc has gotten very slow

quickest-silverOP•2y ago

yes i think so I updated my PR, I hope I tested every combination @Tanner Linsley what do you think about this approach?

dependent-tan•2y ago

I agree with the sentiment that if the from is not specified, the search and params should accept a partial from a union of all possible options. Then in the navigate call, the argument passed to the Update/Reducer functions, would be typed to contain a partial of all the possible union options.

quickest-silverOP•2y ago

why a partial? why not just the union?

dependent-tan•2y ago

My reasoning for a partial from a union of all possible options is that since the from field hasn't been defined, then all keys in the union could/would possibly be undefined. In a DX sense, if all the keys were treated to be 'present', it creates a false sense of security that this field is available (and has any default that could have been set). This however wouldn't be the case at all since the search and path params cannot be guaranteed to have run/validated since the current/from location isn't specified. Maybe partial may not have been the correct term here. Rather just leave it as treating all fields in the union as possibly undefined.

quickest-silverOP•2y ago

let's assume a simple app where there are only two possible search params types:

type T1 = { foo:string ; bar: number}

type T2 = {page:number; filter: string}

type T1 = { foo:string ; bar: number}

type T2 = {page:number; filter: string}

if the union T1|T2 is passed in, the developer needs to add runtime checks which of the two types actually is the one currently present. one of those two types would be the one present and would be fully validated so I see no benefit of Partial<T1> | Partial<T2>

dependent-tan•2y ago

I was thinking more along the lines of Partial<T1 & T2>, since this would be a wild-west scenario (where we don't know where this navigate call is being made from) where all possible types for search and path params could be available just not validated.

genetic-orange•2y ago

I agree with Sean It really could be from any of the routes So all of them are possibilities, and none of them are guaranteed. So honestly it doesn’t matter One of those will give you a *slightly * better experience with type narrowing But it wouldn’t work if you are trying to manipulate a permutation of search parameters that crosses routes. (Which is possible, and I’ve needed it)

quickest-silverOP•2y ago

I think I am still not fully understanding that scenario. Why is "none of them guaranteed"? Let's have a look at a concrete example: https://codesandbox.io/p/devbox/frosty-marco-r39g6f?file=%2Fsrc%2Fmain.tsx%3A159%2C1 The hook useFoo is used in all of the 4 routes. At runtime, the search params would be either

 {
   foo: number;
   filter?: string | undefined;
 }

 {
   foo: number;
   filter?: string | undefined;
 }

{
 foo: "x" | "y";
 bar: number;
}

{
 foo: "x" | "y";
 bar: number;
}

{
}

{
}

But never the intersection of these types nor a partial. Can you please explain this in more detail: "But it wouldn’t work if you are trying to manipulate a permutation of search parameters that crosses routes. (Which is possible, and I’ve needed it)"

genetic-orange•2y ago

I’m mostly describing having the flexibility to read/alter search params as if they are all potentially available at the same time.

quickest-silverOP•2y ago

but if you need a specific param in a route, why is it not part of the validation scheme (even if optional)? could we make this opt in? e.g. offer an intersection type that the user would cast to, but the default would be the union? this would solve the slowness issue of the typescript Compiler and still allow this kind of flexibility

genetic-orange•2y ago

Yeah That'd be fine PR it and let's ship this baby

quickest-silverOP•2y ago

I just looked at my PR (https://github.com/TanStack/router/pull/867) I did not remove the FullSearchSchema type, so this "escape-hatch" type still exists so I think we are good to go

genetic-orange•2y ago

Cool Do you have merge permissions?

quickest-silverOP•2y ago

nope not yet 🙂

genetic-orange•2y ago

Merged

quickest-silverOP•2y ago

great, I'd happily accept merge permissions for the future 😉

genetic-orange•2y ago

Get your invite?

dependent-tan•2y ago

@Manuel Schiller Could you take a look at this bug report? It looks like we have typescript regression with the 1.0.8 release. https://github.com/TanStack/router/issues/908 https://discord.com/channels/719702312431386674/1191910928476819566

GitHub

Broken types since the 1.0.8 release · Issue #908 · TanStack/router

Describe the bug As of the release for 1.0.8, the types are broken on the existing kitchen sink examples. This bug report is only at the typescript inference level. I have not observed any broken f...

quickest-silverOP•2y ago

did you change anything in the kitchen sink example? just want to look at the code in vs code instead of the online IDE

dependent-tan•2y ago

Yes, I modified the existing errors in the kitchen sink example with the same ones I was seeing in my codebases. You could download the Stackblitz example files to view it locally

quickest-silverOP•2y ago

sure, just want to know why on stackblitz the error on line 76 does not occur

dependent-tan•2y ago

Does it not happen locally?

quickest-silverOP•2y ago

screenshot is from VS code, there I see an error I don't see on stackblitz strange

dependent-tan•2y ago

None of the errors in my local codebase match it either... Provided you haven't made any changes to the Router codebase yet... This may just be one of those lovely typescript being typescript moments.

quickest-silverOP•2y ago

ah yes, I was on another branch... my bad

quickest-silverOP•2y ago

this fixes the router.preload and loader({navigate}) issue: https://github.com/TanStack/router/pull/910

GitHub

fix: fix "Type 'string' is not assignable to type 'undefined'" by s...

in loader.navigate / router.preload also updated kitchen sink example fixes #908

quickest-silverOP•2y ago

i also updated the kitchen sink example, can you please have a look @Sean Cassiere ?

dependent-tan•2y ago

Will do Is the navigation API being changed here? The ability to absolutely define the from property isn't something that is always available. Navigation on button presses and links seems like something that should be possible without knowing where it is being called from. Because if this is only enforced on the type-level, then it encourages devs to simply lie to the compiler to get a build to pass through. Whilst if it is enforced at runtime it comes back to the issue where not everyone knows where this is being called from. Off the top of my head, an example of such a component is a user-avatar with a dropdown of navigation options. This component is often rendered across multiple routes (if not the whole application), where it doesn't seem feasible to use this approach.

genetic-orange•2y ago

This is why either from OR strict: false is required

dependent-tan•2y ago

Not able to full getting it running locally... Some pnpm issue on my machine.

quickest-silverOP•2y ago

what's the pnpm issue?

dependent-tan•2y ago

Honestly not even sure anymore... Planned to troubleshoot over the weekend. Will this be the default option? Or the navigation caller explicitly asking the user to choose one or the other. Because without it throwing an error asking for user intervention, its something that could very easily be missed.

quickest-silverOP•2y ago

which "navigation API" in particular? useNavigate and <Link />? or something else

dependent-tan•2y ago

Yup those two are the most common. Though I think the API surface of loader.navigate, <Navigate />, and useRouter().preloadRoute also should be considered here.

quickest-silverOP•2y ago

you don't have to specify from . if you don't, then the "prev" search type will be the union of all possible search types. but that's about it v 1.0.9 is now released with the above mentioned fixes so no need to build yourself

genetic-orange•2y ago

Nah

quickest-silverOP•2y ago

at least in the dreamland of typescript types 😄

genetic-orange•2y ago

😂

Gaming

Programming

FullSearchSchema - why do we need it at all?

Did you find this page helpful?