fredericoo / react-router-typesafe

just the way you wanted React Router to work with TypeScript.
103 stars 3 forks source link

Cannot use defer on a loader with typesafeBrowserRouter #16

Open stevan-borus opened 5 months ago

stevan-borus commented 5 months ago

I have a loader with defer:

import { defer } from 'react-router-typesafe';
...

export const dashboardLoader = (queryClient: QueryClient) => () => {
  return defer<{
    dashboardInfo: Promise<DashboardType>;
  }>({
    dashboardInfo: queryClient.ensureQueryData(dashboardInfoQueries.info()),
  });
};

and I have a RouteObject like this in typesafeBrowserRouter:

{
   index: true,
   loader: dashboardLoader(queryClient),
   async lazy() {
         const { Dashboard } = await import('@/views/dashboard/Dashboard');

         return { element: <Dashboard /> };
   },
},

I get an error: Type () => DeferredData<{ dashboardInfo: Promise<DashboardType>; }> is not assignable to type NarrowKeys<boolean | LoaderFunction<any> | undefined>

This doesn't happen when I am using a regular loader without defer.

Oh, and when I just switch from typesafeBrowserRouter to createBrowserRouter everything works just fine since then, loader doesn't have that NarrowKeys type.

fredericoo commented 5 months ago

could I ask you to try to refactor your loader to the following? I believe it's the fact returning defer isn't overlapping with the LoaderFunction type. I’ll have a deeper look but this may do the trick:

import { defer, makeLoader } from 'react-router-typesafe';
...

export const dashboardLoader = (queryClient: QueryClient) => makeLoader(() => {
  return defer<{
    dashboardInfo: Promise<DashboardType>;
  }>({
    dashboardInfo: queryClient.ensureQueryData(dashboardInfoQueries.info()),
  });
});

the single change here is makeLoader: it’ll auto-add a satisfies keyword to make sure you’re returning a loader-compatible value. You may get a ts error if that defer doesn't match, which is my main bet!

on a personal note, refrain from declaring the generic parameter on the defer function if you can, and rely on the return of the function you have — it should infer just fine, and be more refactorable

stevan-borus commented 5 months ago

I figured out that I get this error on a regular loader too since I moved it outside the lazy loaded module.

makeLoader doesn't solve the problem on both of those loaders. The funny thing is that the app works fine, I just get that typescript error.

Related to the typing of the defer definitely: I see that it is inferring the type now correctly where I useLoaderData after I removed the declared generic. I put it there because when I hover over dashboardLoader it gives my this: function dashboardLoader(queryClient: QueryClient): () => DeferredData<{dashboardInfo: any}> if there is no declared generic. With it I get function dashboardLoader(queryClient: QueryClient): () => DeferredData<{dashboardInfo: DashboardType}> when I hover over it. But it doesn't matter apparently.

fredericoo commented 5 months ago

and you’re saying it doesn’t error on a regular createBrowserRouter call?

could you make a minimal reproduction? I’d love to fix that

stevan-borus commented 5 months ago

and you’re saying it doesn’t error on a regular createBrowserRouter call?

Yup

I made this real quick.. not sure what is going on with the type of typesafeBrowserRouter in the sandbox. Don't have the time now to figure it out.

Here is the repro: https://codesandbox.io/p/sandbox/typesafe-router-c4gryh

stevan-borus commented 5 months ago

When I define the generic on makeLoader with LoaderFunction<any> it works and I don't get the error anymore..

Like this:

export const dashboardLoader = (queryClient: QueryClient) =>
  makeLoader<LoaderFunction<any>>(() => {
    return defer({
      dashboardInfo: queryClient.fetchQuery(dashboardInfoQueries.info()),
    });
  });

Edit: but then useLoaderData<ReturnType<typeof dashboardLoader>>() returns {} | null and I can't use the promise so it's not really a solution

stevan-borus commented 5 months ago

Found out what the actual issue is and I've changed the code in the codesandbox repro.

As you can see, I get the error when I lazy load only the component. If I don't lazy load anything or lazy load both the component and loader then everything works correctly.

So these versions work:

{
   index: true,
   async lazy() {
         const { Dashboard, dashboardLoader } = await import('@/views/dashboard/Dashboard');

         return { element: <Dashboard />, loader: dashboardLoader(queryClient) };
   },
},

and

{
   index: true,
   element: <Dashboard />, 
   loader: dashboardLoader(queryClient)
},

but this one errors:

{
   index: true,
   loader: dashboardLoader(queryClient),
   async lazy() {
         const { Dashboard } = await import('@/views/dashboard/Dashboard');

         return { element: <Dashboard /> };
   },
},
fredericoo commented 5 months ago

hey mate,

appreciate the repro, I’m currently pretty sick so will be back next week. Have you had a look at the React Conf talk by Ryan? it looks like this library will be implemented internally as well, I’m really happy by that!

stevan-borus commented 4 months ago

I hope you are well now.. watched it a couple of days ago. I started migrating an app just before to the new data router so I can slowly transfer it to remix SPA and SSR after that, so I am really excited about this 😃