TanStack / router

🤖 Fully typesafe Router for React (and friends) w/ built-in caching, 1st class search-param APIs, client-side cache integration and isomorphic rendering.
https://tanstack.com/router
MIT License
7.19k stars 486 forks source link

types: expose `BeforeLoadFn` type #1527

Closed milhamm closed 2 weeks ago

milhamm commented 2 weeks ago

I encountered this issue when creating a function that needs the BeforeLoadFn types. I also noticed that other types such as RouteLoaderFn are exposed.

This PR is to expose the BeforeLoadFn type.

SeanCassiere commented 2 weeks ago

@schiller-manuel all good with exposing this type?

nx-cloud[bot] commented 2 weeks ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b89857cccbec3018f6b8cde249985b7cb3b4490c. 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


✅ Successfully ran 2 targets - [`nx affected --targets=test:format,test:eslint,test:unit,test:build,build --parallel=3`](https://cloud.nx.app/runs/YSFt43cyGg?utm_source=pull-request&utm_medium=comment) - [`nx affected --target=test:types --exclude=examples/**`](https://cloud.nx.app/runs/DWDRtYlFbo?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

schiller-manuel commented 2 weeks ago

I am not sure how you would use this type outside of a route definition and still have all types be correctly inferred. Can you show an example?

SeanCassiere commented 2 weeks ago

No objections from me about pushing this through.

@milhamm could you respond to Manuel's question? If he's fine with it, this can be pushed through.

milhamm commented 2 weeks ago

Sure @schiller-manuel @SeanCassiere.

So this is my example. In my case, It is not fully inferred, but I have a function that strictly handles only routes with search name with a typed context. Now the beforeLoadWithNameSearch has the typing for the search and also context.

type WithNameSearch = {
  name?: string
}

const beforeLoadWithNameSearch: BeforeLoadFn<
  WithNameSearch,
  AnyRoute,
  any,
  RouteContext,
  MyRouterContext
> = ({ search, context }) => {
  console.log(search.name)
  console.log(context.user.name)
}

export interface MyRouterContext {
  user: {
    name?: string
  }
}

export const RootRoute = createRootRouteWithContext<MyRouterContext>()({
  component: () => {},
  notFoundComponent: () => {
    return <p>This is the notFoundComponent configured on root route</p>
  },
})

export const Route = createFileRoute('/posts/$postId/deep')({
  loader: async ({ params: { postId } }) => fetchPost(postId),
  errorComponent: PostErrorComponent as any,
  component: PostDeepComponent,
  validateSearch: (search: Record<string, unknown>): { name?: string } => {
    return {
      name: (search.name as string) || undefined,
    }
  },
  beforeLoad: beforeLoadWithNameSearch,
})
schiller-manuel commented 2 weeks ago

hm I don't think this type should be part of the public API, since we might want to modify the types / generics etc. What do you really gain from using the BeforeLoadFn type? wouldn't this be much simpler?


const beforeLoadWithNameSearch = (search : WithNameSearch, context : MyRouterContext) => {
  console.log(search.name)
  console.log(context.user.name)
}

export const Route = createFileRoute('/posts/$postId/deep')({
  loader: async ({ params: { postId } }) => fetchPost(postId),
  errorComponent: PostErrorComponent as any,
  component: PostDeepComponent,
  validateSearch: (search: Record<string, unknown>): { name?: string } => {
    return {
      name: (search.name as string) || undefined,
    }
  },
  beforeLoad: ({search, context}) => beforeLoadWithNameSearch(search, context),
})
milhamm commented 2 weeks ago

@schiller-manuel Yes you are right fair point, I don't see the benefit of exposing those to a public type now that you mentioned it. In fact, your example is the current implementation in my code right now.