remix icon indicating copy to clipboard operation
remix copied to clipboard

Incorrect type from `useLoaderData`

Open nullndr opened this issue 3 years ago • 13 comments

What version of Remix are you using?

1.6.7

Steps to Reproduce

export const loader = async () => {
  return {
    requestedDomains: [
      { domain: "example.com", status: "pending" },
      { domain: "saulgoodman.org", status: "approved" },
      { domain: "example.org", status: "rejected", reason: "Not allowed" },
    ],
  };
};

export default function Foo() {
  const { requestedDomains } = useLoaderData<typeof loader>();
  ...
}

Expected Behavior

the type of requestedDomains should be

SerializeObject<{
    domain: string;
    status: string;
    reason?: string;
}>[]

Actual Behavior

the type of requestedDomains is:

(SerializeObject<{
    domain: string;
    status: string;
    reason?: undefined;
}> | SerializeObject<{
    domain: string;
    status: string;
    reason: string;
}>)[]

See also this discussion where I try some solutions.

nullndr avatar Aug 05 '22 14:08 nullndr

Maybe this is related to TypeScript, but should I use the LoaderData in this case?

Thought we should completely replace it

nullndr avatar Aug 05 '22 14:08 nullndr

I confirm I have the same "bug" of TS types with the tutorial

ygrenzinger avatar Aug 07 '22 13:08 ygrenzinger

Another example:

import { useLoaderData } from "@remix-run/react";
import { json } from "@remix-run/server-runtime";

export function loader() {
  const x = Math.random();

  if (x === 1) {
    return json({ a: "stringA" });
  }

  if (x === 2) {
    return json({ a: 2});
  }

  if (x === 3) {
    return json({ a: 2, b: 2 });
  }

  return json({ c: "stringC" });
}


export default function AppTest() {
  const loaderData = useLoaderData<typeof loader>();
  ...
}

const loaderData: SerializeObject<{
    a: string;
}> | SerializeObject<{
    a: number;
}> | SerializeObject<{
    c: string;
}>

b is never there.

Changing the type of a in the x === 3 case fixes the type

export function loader() {
  const x = Math.random();

  if (x === 1) {
    return json({ a: "stringA" });
  }

  if (x === 2) {
    return json({ a: 2});
  }

  if (x === 3) {
    return json({ a: [2], b: 2 });
  }

  return json({ c: "stringC" });
}


export default function AppTest() {
  const loaderData = useLoaderData<typeof loader>();
  ...
}

```ts
const loaderData: SerializeObject<{
    a: string;
}> | SerializeObject<{
    a: number;
}> | SerializeObject<{
    a: number[];
    b: number;
}> | SerializeObject<{
    c: string;
}>

The conclusion I was able to draw thus far is if an object is returned that has a property of the same type, but an extra property added as well, it doesn't get picked up.

If I take the first example and completely remove the x === 2 case, the b property is picked up and the type if as follows:

export function loader() {
  const x = Math.random();

  if (x === 1) {
    return json({ a: "stringA" });
  }

//   if (x === 2) {
//     return json({ a: 2});
//   }

  if (x === 3) {
    return json({ a: 2, b: 2 });
  }

  return json({ c: "stringC" });
}


export default function AppTest() {
  const loaderData = useLoaderData<typeof loader>();
  ...
}
const loaderData: SerializeObject<{
    a: string;
}> | SerializeObject<{
    a: number;
    b: number;
}> | SerializeObject<{
    c: string;
}>

miniplus avatar Aug 11 '22 11:08 miniplus

Unfortunately I think we're going to continue to get a lot of these weird edge cases. The issue is that Remix is trying to convert your actual data type, with all its various forms, and convert that to what the type would look like if you replaced any non-JSON types with their serialized versions.

They are using a home-grown function to do this, but plan to use Jsonify from type-fest. Even then, I'm not sure it it will every be 100% perfect.

I side-stepped this with remix-typedjson by simply using whatever type that TypeScript thinks the loader is returning. In order to do this, I also wrote my own JSON serialize/deserialize function (similar to superjson) to ensure that non-JSON values like Date, BigInt, etc. are converted back to their native values after parsing.

kiliman avatar Aug 11 '22 15:08 kiliman

Thank you @kiliman, we started using remix-typedjson right away when I saw this.

memark avatar Aug 22 '22 17:08 memark

Can this be included in remix, types returned from remix are unusable. When using prisma, Remix Json: SerializeObject<UndefinedToOptional<Person>>[] Typed Json: Person[]

@kiliman Thanks!

nexneo avatar Oct 21 '22 13:10 nexneo

Sadly, even with remix-typedjson it still doesn't work for me.

export interface LinksGroupProps {
  icon: TablerIcon;
  label: string;
  initiallyOpened?: boolean;
  links?: { label: string; link: string }[];
}

export const loader = async ({request}: LoaderArgs) => {
  const data = {
    main: [
        {
          link: "/taskapp",
          label: "Task App",
          links: [
            {
              link: "/tasks",
              label: "Tasks"
            },
            {
              link: "/processes",
              label: "Processes"
            },
            {
              link: "/cases",
              label: "Cases"
            }
          ]
        },
        {
          link: "/modeler",
          label: "Modeler",
        },
        {
          link: "/admin",
          label: "Admin"
        },
        {
          link: "/idm",
          label: "IDM"
        }
    ],
    side: [
        { icon: IconGauge, label: 'Dashboard' },
        {
          label: 'Market news',
          initiallyOpened: true,
          links: [
            { label: 'Overview', link: '/' },
            { label: 'Forecasts', link: '/' },
            { label: 'Outlook', link: '/' },
            { label: 'Real time', link: '/' },
          ],
          icon: IconNotes,
        },
        {
          label: 'Releases',
          links: [
            { label: 'Upcoming releases', link: '/' },
            { label: 'Previous releases', link: '/' },
            { label: 'Releases schedule', link: '/' },
          ],
          icon: IconCalendarStats,
        },
        { label: 'Analytics', icon: IconPresentationAnalytics },
        { label: 'Contracts', icon: IconFileAnalytics },
        { label: 'Settings', icon: IconAdjustments },
        {
          label: 'Security',
          links: [
            { label: 'Enable 2FA', link: '/' },
            { label: 'Change password', link: '/' },
            { label: 'Recovery codes', link: '/' },
          ],
          icon: IconLock,
        }
    ]
  }
  // return json(data)
  return typedjson(data)
}

export default function Tasks() {
  //const {main, side} = useLoaderData() as LoaderData
  const {main, side} = useTypedLoaderData<typeof loader>()
  console.log("side", side)
  
  return (
    <>
      <HeaderComponent links= {main} />
      <NavbarNested links= {side} />
      <Outlet/>
    </>
  );
}

It seems like complex type like TablerIcon doesn't get serialized and send correctly across the wire.

nduc avatar Oct 25 '22 22:10 nduc

Yes, as explained in remix-typedjson, it only supports a subset of types: Date, BigInt, etc. I'm not sure what your custom types look like, so hard to say if it should serialize or not.

You could also try remix-superjson. It uses superjson to serialize and supports more complex types.

kiliman avatar Oct 25 '22 23:10 kiliman

Thanks for the clarification.

nduc avatar Oct 26 '22 00:10 nduc

@nduc please could you format your code with markdown?

nullndr avatar Oct 26 '22 05:10 nullndr

For the remix tutorial, adding 'unknown' converstion seemed to make TS happy

  const { posts } = useLoaderData() as unknown as LoaderData;

oldmill1 avatar Oct 28 '22 22:10 oldmill1

~~I’m having trouble with data coming straight from a GraphQL API, i.e. data that should already be serialised. Would it be a good idea to be able to allow the user to “opt out” of type serialisation (e.g. via a type argument)?~~

~~I don’t know enough about TypeScript to know if the serialisation problem is easily solvable — but if not, this suggestion seems to be the easiest fix.~~

Edit: please ignore, my issue was elsewhere and has been fixed in the meantime.

lensbart avatar Oct 30 '22 00:10 lensbart

For the remix tutorial, adding 'unknown' converstion seemed to make TS happy

  const { posts } = useLoaderData() as unknown as LoaderData;

For other people's reference, this is force casting: https://www.w3schools.com/typescript/typescript_casting.php

What would be the logical thing to do if done correctly without force casting? Should LoaderData type be updated or useLoaderData() method be updated. Ideally useLoaderData() should not have to go through unknown casting, so curious.

kwantopia avatar Oct 30 '22 16:10 kwantopia

Don't want to sound rude, but a feedback to the Remix Team - Facing the same issue and this has been frustrating! I had heard a lot about Remix but these kinds of errors while trying out its first tutorial makes me not want to use it for projects :(

Pranav2612000 avatar Nov 20 '22 18:11 Pranav2612000

useLoaderData<LoaderData>()

steven-so avatar Nov 21 '22 23:11 steven-so

+1 this is definitely confusing and not clear how best to handle. I would fully expect to be able to do this:

const { user, notes}: { user: User | undefined, notes: Note[]} = useLoaderData<typeof loader>();
// user is type User | undefined
// notes is type Note[]

given a loader function akin to,

export async function loader({ request }: LoaderArgs) {
  const user = await useOptionalUser();
  const notes = user ? await getNotes({ userId: user.id }) : ([] as Note[]);
  return json({ user, notes });
}

quick edit: thinking about this some more, perhaps a prisma-client model could automatically provide a toJSON() serializer ...perhaps it's not json's job to handle this, but having it in the indie-stack example makes it confusing when slight adjustments leads to the SerializeObject<UndefinedToOptional<Note>>[] issue.

uhrohraggy avatar Dec 30 '22 21:12 uhrohraggy

Yeah, I'm not a big fan of the SerializeObject type. That's why I wrote remix-typedjson. It will automatically convert non-JSON types back into their native types, so you can use the actual types instead of the JSON-converted ones.

kiliman avatar Jan 02 '23 16:01 kiliman

Is there any update on whether a solution akin to remix-typedjson will be introduced within Remix?

binjamil avatar Mar 11 '23 23:03 binjamil

So a lot has passed since I opened this issue and I read all your comments.

The snippet I shared at the start was a simplified version of a loader I actually have in a project for Shopify, in this time I enhanced the project, updating also remix to 1.14.1, so I would like to share with you some of my thoughts.

For this, take the following loader for an embedded Shopify app:

export const loader = async ({ request }: LoaderArgs) => {
  const url = new URL(request.url);
  const shopifyDomain = url.searchParams.get("shop");
  const host = url.searchParams.get("host");

  if (shopifyDomain && host) {
    const session = await shopSession.getSession(request.headers.get("Cookie"));
    if (session.get("shopifyDomain") === shopifyDomain) {
      const shop = await findShopInAuth({ shopifyDomain });
      if (shop) {
        const apiKey = Env.get("SHOPIFY_API_KEY");
        return {
          success: true,
          apiKey,
          host,
        };
      }
    }

    throw redirect(`/api/auth?shop=${shopifyDomain}&host=${host}`);
  }

  return {
    success: false,
  };
};

The return type of this loader is

Promise<{
    success: boolean;
    apiKey: string;
    host: string;
} | {
    success: boolean;
    apiKey?: undefined;
    host?: undefined;
}>

Please, keep in mind that this is just how typescript infer the return type.

So you can't correctly discriminate this union:

export default function EmbeddedLayout() {
  const loaderData = useLoaderData<typeof loader>();
  const locale = useLocale();

  const { ready } = useTranslation("translation", { useSuspense: false });

  if (ready) {
    return loaderData.success ? (
      <EmbeddedApp {...loaderData}> // error
        <Outlet />
      </EmbeddedApp>
    ) : (
      <AppProvider i18n={locale} linkComponent={Link}>
        <Outlet />
      </AppProvider>
    );
  }

  return <></>;
}

type EmbeddedAppProps = React.PropsWithChildren<{
  apiKey: string;
  host: string;
}>;

declare function EmbeddedApp({ children, apiKey, host }: EmbeddedAppProps);

This raise an error since loaderData is of type:

SerializeObject<UndefinedToOptional<{
    success: boolean;
    apiKey: string;
    host: string;
}>> | SerializeObject<UndefinedToOptional<{
    success: boolean;
    apiKey?: undefined;
    host?: undefined;
}>>

So, should we use a LoaderData type? Well, you could:

type LoaderData =
  | { success: true; apiKey: string; host: string }
  | { success: false };

export default function EmbeddedLayout() {
  const loaderData = useLoaderData<LoaderData>();
  const locale = useLocale();

  const { ready } = useTranslation("translation", { useSuspense: false });

  if (ready) {
    return loaderData.success ? (
      <EmbeddedApp {...loaderData}> // all good
        <Outlet />
      </EmbeddedApp>
    ) : (
      <AppProvider i18n={locale} linkComponent={Link}>
        <Outlet />
      </AppProvider>
    );
  }

  return <></>;
}

but from my little experience I don't see this approach scalable, since if the loader has to return another object, you should also update the LoaderData type, otherwise you will get some errors.

I don't like this approach, so instead I started to use as const on returns:

return {
  success: true,
  apiKey,
  host,
} as const;

...

return {
  success: false,
} as const;

From this typescript infer the following type:

Promise<{
    readonly success: true;
    readonly apiKey: string;
    readonly host: string;
} | {
    readonly success: false;
    readonly apiKey?: undefined;
    readonly host?: undefined;
}>

That is a truly discriminated union, and now loaderData is

SerializeObject<UndefinedToOptional<{
    readonly success: true;
    readonly apiKey: string;
    readonly host: string;
}>> | SerializeObject<UndefinedToOptional<{
    readonly success: false;
    readonly apiKey?: undefined;
    readonly host?: undefined;
}>>

This works great, no force casting with unknown and no LoaderData type.

A little enhancement from Remix would be to remove from the type keys with undefined as value, since they won't exists in the returned json, but I think it is ok now.

@miniplus this could also fix your issue, however I don't know why you never see b, I use typescript 4.9.5, and the following typescript works:

function loader() {
  const x = Math.random();

  if (x === 1) {
    return { a: "stringA" } as const;
  }

  if (x === 2) {
    return { a: 2 } as const;
  }

  if (x === 3) {
    return { a: 2, b: 2 } as const;
  }

  return { c: "stringC" } as const;
}

export type Prettify<T> = { [K in keyof T]: T[K] } & {};

type RemoveUndefined<T> = Prettify<{ -readonly [K in keyof T as T[K] extends undefined ? never : K]: T[K]}>

const bar: RemoveUndefined<ReturnType<typeof loader>> = loader();

The type of bar is:

{
    a: "stringA";
} | {
    a: 2;
} | {
    a: 2;
    b: 2;
} | {
    c: "stringC";
}

Here is a playground

@uhrohraggy from the type you posted, user should be User | null, and not User | undefined, null will be correctly serialized into the json, undefined won't

nullndr avatar Mar 16 '23 10:03 nullndr

I have Remix 1.14.3 and Typescript 4.9.5 in my project but this approach doesn't work for me in case I return nested structures from loader:

return {
  foo: {
    bar: "it's a string" 
  }
}

For flat structures it does work also without as const.

kwiat1990 avatar Mar 27 '23 15:03 kwiat1990

Hey @kwiat1990, could you provide more details for your problem please?

nullndr avatar Mar 27 '23 15:03 nullndr

I think I didn't quite get the last part of your example in which you call loader(). How it should be using along with useLoaderData?

kwiat1990 avatar Mar 27 '23 15:03 kwiat1990

@kwiat1990 oh sorry, that was a simple typescript example, here is how you should translate it to remix:

export const loader = () => {
  const x = Math.random();

  if (x === 1) {
    return { a: "stringA" } as const;
  }

  if (x === 2) {
    return { a: 2 } as const;
  }

  if (x === 3) {
    return { a: 2, b: 2 } as const;
  }

  return { c: "stringC" } as const;
};

export default function Index() {
  const loaderData = useLoaderData<typeof loader>();

  if ("a" in loaderData) {
    loaderData.a; // "stringA" | 2
  }

  if ("b" in loaderData) {
    loaderData; // { a: 2; b: 2; }
  }

  if ("c" in loaderData) {
    loaderData; // { c: "stringC"; }
  }

  return <div></div>;
}

As I said, the loaderData type will encapsulate the actual data in SerializeObject<UndefinedToOptional<...>>, so if you want to remove undefined keys you have to type it like the following:

type Prettify<T> = { [K in keyof T]: T[K] } & {};

type RemoveUndefined<T> = Prettify<{
  -readonly [K in keyof T as T[K] extends undefined ? never : K]: T[K];
}>;

const loaderData: RemoveUndefined<Awaited<ReturnType<typeof loader>>> =
  useLoaderData<typeof loader>();

Or better, write your own useLoaderData hook like this:

function useLoaderData<T extends () => any>(): RemoveUndefined<
  Awaited<ReturnType<T>>
> {
  return useRemixLoaderData<T>();
}

nullndr avatar Mar 28 '23 06:03 nullndr

OK then, it seems I have made everything correctly but like I said before, with this Typescript throws following error:

Type 'SerializeObject<UndefinedToOptional<{ readonly article: { readonly content: string; readonly readTime: string; readonly author: Author; readonly category: Category; readonly createdAt: string; readonly id: string; ... 6 more ...; readonly cover?: Image | undefined; }; readonly comments: Comment[]; readonly commentsC...' is not assignable to type '{ article: { readonly content: string; readonly readTime: string; readonly author: Author; readonly category: Category; readonly createdAt: string; readonly id: string; ... 6 more ...; readonly cover?: Image | undefined; }; comments: Comment[]; commentsCount: number; pagination: Pagination; relatedArticles: Article[...'.
  The types of 'article.author.avatar' are incompatible between these types.
    Type 'SerializeObject<UndefinedToOptional<Image>> | undefined' is not assignable to type 'Image | undefined'.
      Type 'SerializeObject<UndefinedToOptional<Image>>' is not assignable to type 'Image'.
        Types of property 'createdAt' are incompatible.
          Type 'string | undefined' is not assignable to type 'Date | undefined'.
            Type 'string' is not assignable to type 'Date'.ts(2322)

One of the things being returned from the loader and what's causing Typescript error is article, which looks like this:

export interface Article {
  author: Author;
  category: Category;
  content: string;
  createdAt: string;
  id: string;
  lead: string;
  publishedAt: string;
  slug: string;
  tags: Tag[];
  title: string;
  updatedAt: string;
  cover?: Image;
  readTime?: string;
}

kwiat1990 avatar Mar 28 '23 07:03 kwiat1990

@kwiat1990 from the error Typescript raised the problem is in the type of article.author.avatar.createdAt that is serialized correctly as string since initially it is of type Date, in order to fix this issue you have to recreate a Date object from it.

A possible solution (although not elegant) could be the following:

export default function() {
  const loaderData = useLoaderData<typeof loader>();

  return <Component 
            article={
              { 
                ...loaderData,
                author: { 
                  ...loaderData.author, 
                  avatar: { 
                    ...loaderData.author.avatar,
                    createdAt: new Date(loaderData.author.avatar.createdAt) 
                  }
                }
              }
         }/>
}

nullndr avatar Mar 28 '23 08:03 nullndr

In any place or component I expect a Date object. For that I have a helper function, which formats a date from a Date object or a string. In data from loader everything date-related is a string.

I get mentioned error at this place in my code:

const loaderData: RemoveUndefined<Awaited<ReturnType<typeof loader>>> =
    useLoaderData<typeof loader>();

The bottom line is that with the changes how Remix types loader data a lot of things got broken and currently I don't see any easy way in order to fix it. The typedJson seems to cover only simply types.

kwiat1990 avatar Mar 28 '23 15:03 kwiat1990

@kwiat1990 oh sorry, the problem here is that the type from useLoaderData can not be assigned to loaderData since it is of type RemoveUndefined<Awaited<ReturnType<typeof loader>>>.

For this, you should also pass ReturnType<typeof loader> to SerializeObject<UndefinedToOptional<...>>

nullndr avatar Mar 28 '23 16:03 nullndr

Thanks for clarification. It feels a bit counterproductive, sort of, to fight against a framework of your choice. I would rather stick to my own interfaces/types for loader data, which I need to write by myself than to use more and more wrappers to get rid of Remix types.

kwiat1990 avatar Mar 29 '23 12:03 kwiat1990

The issue here is that Remix is trying NOT to lie to you by specifying the actual result type you're getting, which may differ from what you returned from your loader due to JSON conversion.

And yes, it can be annoying, especially for nested objects. I'm sure it's difficult to have some automated way to infer this result type for countless return values that will work well in all cases.

That's why I punted, and instead of returning a JSON value, I returned my typed JSON, which automatically converts back to its native form. Then I can simply use the actual type from the loader inferred by TypeScript.

kiliman avatar Mar 29 '23 13:03 kiliman

My issue with this behavior is that I need to manage to write some Typescript wrapper for this to work or install yet another dev dependency and hope it works with my custom types. In both cases I'm not the happiest guy in the world. I would really prefer that Remix says: hey, you need type those things on your own. We can't.

kwiat1990 avatar Mar 30 '23 07:03 kwiat1990

I think the solution here is using SerializeFrom from @remix-run/server-runtime.

e.g.

export default function PostList({ posts }: { posts: SerializeFrom<Post[]>; }) {
  return (
    <div>
        Posts!
    </div>
  );
}

graysonhicks avatar Apr 23 '23 17:04 graysonhicks