Hi @colinhacks,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

How about restricting json<Data>() type to only extend from JSONValue. This will ensure the developer won't send non-JSON values like Date or BigInt

type JSONValue =
    | string
    | number
    | boolean
    | { [x: string]: JSONValue }
    | Array<JSONValue>;

How about restricting json<Data>() type to only extend from JSONValue. This will ensure the developer won't send non-JSON values like Date or BigInt

type JSONValue =
    | string
    | number
    | boolean
    | { [x: string]: JSONValue }
    | Array<JSONValue>;

I suggested this in #1254 but I'd want confirmation from the team before I implement it. It's possible it'll break some codebases if the return type of loader is unknown or contains unknown:

type JSON =
  | string
  | number
  | boolean
  | {[x: string]: JSON}
  | Array<JSON>;

const arg: JSON = {asdf: 'asdf' as unknown};
//    ^ not assignable to type JSON

Not sure how common this is but I don't think it's a big problem. Worth the improved type safety IMO.

Can we extend this to work with useActionData as well?

I'd suggest useFetcher, but that seems a bit more error-prone; it's a lot easier to mismatch the fetcher action with the loader function you'd have to pass in, but I'd be open to it if others feel different

Oops got busy with travel and weddings! Back in the swing of things now.

It's for the useLoaderData generic to be able to get the type out of typeof loader.

Exactly 🤙

Next question, what happens when the loader returns a redirect or a new Response()?

Good question. If you don't use return json(...), inference isn't possible and the return type of useLoader<typeof loader>() will be any. This is true for both return redirect(...) and return new Response({ ... }).

One interesting case: if the user throws the redirect (which I believe is recommended?), that code path isn't factored into the loader's inferred return type. This is how TypeScript works, not something I implemented. This is desirable, since you can throw redirects without messing up loader type inference.

export const loader = async (args: LoaderArgs) => {
  const sessionCookie = args.request.headers.get("cookie");

  if (!sessionCookie) {
    throw redirect("/login");
  }

  return json({ ok: true });
};

export default function Home(){
  const data = useLoaderData<typeof loader>();  // { ok: boolean }
}

Unfortunately conditionally returning a redirect makes type inference impossible:

export const loader = (args: LoaderArgs) => {
  if (Math.random()) return redirect("/");
  return json({ ok: true });
}

export default function Home(){
  const data = useLoaderData<typeof loader>();    // any
}

You may have assumed that the return type of this loader is Response | TypedResponse<{ok: boolean}> but because TypedResponse<T> is a subtype of Response, TypeScript collapses this union type down to simply Response. And when the loader returns a plain untyped Response, useLoaderData<typeof loader>() will return any.

For this reason, the docs should always recommend throwing redirects, IMO.

Regarding redirect, would it work here to return TypedResponse<never>? In a union TypedResponse<never | SomeType>, the never goes away, and you get TypedResponse<SomeType>. I rely on this in my own version of typed helpers. I also think it's intuitive; if you navigate away from the page, that data will never actually arrive to the page.

So, to be clear, we'd be fine if we do:

export const loader = (args: LoaderArgs) => {
  if (Math.random()) throw redirect("/");
  return json({ ok: true });
}

I think I can live with that. I'm on a family trip right now (just hitting GitHub on some down-time), but when I get the chance I'll talk with Ryan and Michael about this. Thanks for your patience!

Ooh good idea @itsMapleLeaf! I updated the PR to use that solution. This way, users don't need to remember to throw redirects, and type inference will work no matter what. Seems unambiguously better IMO.

when I get the chance I'll talk with Ryan and Michael about this. Thanks for your patience

🤙 no rush

Thanks, @colinhacks! I took a rudimentary attempt at this earlier this year and will be glad to see this get merged.

Let me know if you need any help with testing edge cases.

Thanks, @colinhacks! I took a rudimentary attempt at this earlier this year and will be glad to see this get merged.

Let me know if you need any help with testing edge cases.

Oh wow, hadn't seen that discussion! I see you also chose a simplified typeof loader approach (without Awaited<ReturnType<...>>) and renamed DataFunctionArgs to LoaderArgs. We're definitely on the same wavelength 😛

I'm convinced that InferLoaderData is the right approach, but I'm waffling between some options for how it gets used:

// 1. explicit type cast
useLoaderData() as InferLoaderData<typeof loader>

// 2. generic, uses `InferLoaderData`
useLoaderData<InferLoaderData<typeof loader>>()

// 3. for convenience, generic uses `typeof loader` and `useLoaderData` will use `InferLoaderData` implicitly
useLoaderData<typeof loader>()

I like the terseness of (3), but I think I lean towards (1) because AFAIK useLoaderData isn't actually validating that the data it retrieves matches the expected type. In short, (2) and (3) feel like they're hiding a lie, whereas with (1) the user clearly sees that a cast is happening and that the data isn't validated, just cast.

@colinhacks I know you've expressed some opinions on this already, but the nuance I'm interested in isn't that a lie is happening, its whether that lie is hidden or clearly visible to the user.

@MichaelDeBoey @mattpocock I'm also interested in hearing which option 1, 2, or 3 you think is best, or more specifically, if you think a generic that smuggles in a type cast is nasty or fine or something else.

I like useLoaderData<typeof loader>() (3) the most tbh.

In essence, as a user of the useLoaderData hook, I don't care about type inference & if the actual returned value checked against the type or not.
All I want to do is define my loader & tell the hook "this is my loader, do whatever you need to do with it to give me the correct return type of the useLoaderData hook".

I would suspect the return type to be exactly as what's happening behind the scenes (so only valid serialized return values from JSON.stringify, no Dates or any other types).

Nr. 1 is my least favorite tbh.
We're used to pass return types as a generic all over the ecosystem, so not being able to do that anymore would be a step backwards imo.
Besides that, it would be a breaking change to remove the option of passing a generic as well.

Having talked about this with Pedro, I'm convinced option 1 is the option I "hate the least" so I vote that one.

Seems you're trying to pick an API that actively encourages users to distrust the inferred types. If that's your goal then we are definitely not trying to accomplish the same thing. 🙃

A few points relating to this discussion.

a) DX and adoption

First things first: there's no difference in "typesafety" among those three options. They all achieve the same thing in different ways. TypeScript users who really care about full typesafety will use whatever syntax you provide, the question is just a matter of how verbose/onerous the experience will be. My primary concern isn't whether or not the "lie" is obvious to the user; I'm far more worried about users not bothering with typing at all because the idiomatic approach to getting types is too annoying.

b) Clients should trust the server

The scenario I've been imagining is a user who's using a typesafe ORM to fetch data in their loader. The return type of these ORM calls is generally trustworthy and probably enforced at runtime internally by the ORM. The loader code runs on the server in a trusted environment. Generally the client-side code should consider server code and it's inferred types trustworthy.

That said, it's very possible users will use type annotations that make the static type inconsistent with runtime:

export const loader = (args: LoaderArgs)=>{
  const data: unknown = { message: 5 };
  return data as { message: string };
};

But in this scenario, they're already using an explicit type cast — as {message: string}. It seems you favor (1) because the presence of an explicit type cast indicates to the user that they are forcing TypeScript to believe something that may not be true. That's very fair - I hate type annotations with a fiery passion for this exact reason. Since the user is already using a cast in the loader, why require it again in useLoaderData? Types don't come out of nowhere in TypeScript - they are either inferred or "assigned" with a cast. If a user is writing buggy code that leads to static-runtime disagreement in the loader return type, they already used some sort of cast in their loader code and can thus be considered duly informed of the potential invalidity, no? Requiring another cast on the client is redundant.

c) TS utility types 😑

Admittedly InferLoaderType is much cleaner than InferGetServerSidePropsReturnType but Next.js utility types for doing type inference really starting grating on me over time. It just felt like I was duct-taping together the static and runtime parts of my codebase.

Summarizing a conversation I had with @ryanflorence about this:

The root cause here is that loader is an implicit input for useLoaderData. So modeling that directly in the function signature would be useLoaderData(loader). But we want to call the loader outside of rendering (that's like the whole point of Remix!), so its called before useLoaderData. That means we can't directly infer types from loader through useLoaderData without a type cast or using any somewhere in the data flow from loader to useLoaderData.

One thought is that for option (3), type purists could still omit the generic to get unknown back from useLoaderData:

let data = useLoaderData() // unknown
let product = productSchema.parse(data) // use `zod` or `invariant` or another runtime checking library to get type safety

Unfortunately if we use (2) or (3), for purists that are just following docs/examples that means TS will never warn them that data from useLoaderData was never validated/parsed and that they should do so themselves.

let product = useLoaderData<typeof loader>() // TS won't warn purists that we're never validating this data to make sure it aligns with the return type of `loader`

But for semi-purists or users who don't care as much about strict, 100% type safety, it'd be nice if they weren't required to import InferLoaderData and do the incantation of useLoaderData() as InferLoaderData<typeof loader>.

Plus loader is defined right there next to the usage of useLoaderData AND usage of the generic seems very error-averse: just do useLoaderData<typeof loader>.

This all makes me lean toward (3) as long as:

• we allow omission of the generic causing useLoaderData to return unknown
• we clearly document that purists can omit the generic and use a runtime validation/parsing library to get strict type safety

I think the best place for documenting that the generic can be omitted is in the jsdoc for useLoaderData since purists will be prone to look at the definition for useLoaderData.

P.S. Sorry @kentcdodds! Just after I got more convinced of (1) when talking to you, I waffled one more time to (3) 😅

we allow omission of the generic causing useLoaderData to return unknown

It's any currently but could be changed to unknown. This would break a lot of code though - loaderData would require explicit type casting before it can really be used anywhere, since unknown isn't assignable to anything. You can also access properties on an any value but not unknown.

("asdf" as any).whatever[6].stuff; // works
('asdf' as unknown).whatever; // SyntaxError

we clearly document that purists can omit the generic and use a runtime validation/parsing library to get strict type safety

True purists are the ones who are most likely to hate a syntax that involves a type cast. The need for a type cast indicates that you are either a) failing to properly infer a type upstream or b) runtime validation with Zod, etc is needed. The purist approach would involve using a typesafe ORM in their loader, which provides trustable types. It would also then be redundant to revalidate loaderData on the client.

But in any case this is definitely a documentable pattern. (Aside: You wouldn't want to validate inline since validation can be CPU-intensive; you'd use useMemo here.)

Has that patch been published yet?

It's probably in v0.0.0-nightly-c3ec279-20220708

I think I have found a bug 🐛

Using the Final API provided in the descriptions of this PR, just include the return { count: 1 }; in the loader function. The loader function return type is Promise<TypedResponse<{ hello: string }> | { count: number }>.

The data returned by useLoaderData<typeof loader> is expected to be inferred as { hello: string } | { count: number } but it is incorrectly being included with Response fields.

import {LoaderArgs} from "@remix-run/node";

const loader = async (args: LoaderArgs)=>{
  return { count: 1 };
  return json({ hello: "data" }); // TypedResponse<{hello: string}>
}

export default App(){
  // ⚠️ data is not inferred correctly here
  const data = useLoaderData<typeof loader>()
  return <div>{data}</div>
}

Another reason why I prefer to type my loader data explicitly. I just think we're expecting too much from TypeScript here.

@lemol you mean you have two return statements in your loader? Why would you do that?

@kiliman good news is that even with this feature you can continue to manually type everything you like.

Personally I think we're not expecting too much and this is going to be a huge productivity boost.

Looks like this is breaking useLoaderData() as LoaderData which we need to support at least until the next breaking change:

Error: app/routes/notes/$noteId.tsx(36,16): error TS2352: Conversion of type '{ note: { id: string; title: string; body: string; createdAt: string; updatedAt: string; userId: string; }; }' to type 'LoaderData' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.

How can we make sure we don't break that?

@lemol you mean you have two return statements in your loader? Why would you do that?

That was just to illustrate when there is a mix of returned types. Actual useful example may be returning conditionally:

const loader = async (args: LoaderArgs)=>{
  if (some condition) {
    return { count: 1 };
  }

  return json({ hello: "data" }); // TypedResponse<{hello: string}>
}

@lemol I would consider the bug in this example to be not returning the count object in the json object. It properly types the function if you fix it.

const loader = async (args: LoaderArgs)=>{
  if (some condition) {
-    return { count: 1 };
+    return json({ count: 1 });
  }

  return json({ hello: "data" }); // TypedResponse<{count: number} | {hello: string}>
}

P.S. @colinhacks this works really well for me on the dev branch, can't wait to see this released! 🥳

Good catch @joshmedeski, just came to say the exact thing!

@kentcdodds This is interesting. The loader data that the hook returns wouldn't have a Date type, so I'm not sure if it's a breaking change or a fix. The error is this:

The types of 'note.createdAt' are incompatible between these types.
    Type 'string' is not comparable to type 'Date'.

which is correct, depending on note.createdAt to be a Date would actually be an error. If there are people who depend on useLoaderData() as LoaderData, this is a time bomb. The correct usage would be useLoaderData() as SerializeType<LoaderData>.

But real life is messy and it's not great for people to update Remix and have type errors (even for wrong things). I guess it would be a breaking change that could wrap LoaderData with SerializeType as a codemod. 🤷

@joshmedeski is correct. This is a new feature which you opt-into so it's non-breaking. We can say: "if you want to use this feature (typeof loader) then you must use json" which we want to encourage anyway and in v2 we'll probably remove the ability to return a plain object.

@frontsideair

real life is messy

Correct. It's embarrassing that I didn't notice that in the actual error and also that we've gone so long having that time bomb in our starters (FWIW, most people probably delete the $noteId.tsx route post-generation anyway, but still).

I think what we should do in this situation is push forward and we'll call out this in the notes. In a world where bug fixes can break people's CI, every new version could be considered a major version bump. I don't want to live in a world like that, especially when it's just CI and not production code since it's just TypeScript compilation that will be affected.

This has been released in v1.6.5 🎉

However it's not working as expected in remix-run/indie-stack#122 🤔

Ah, I didn't realize that we won't be using LoaderFunction anymore. At first I wasn't excited about this. I'm still not. But I'm willing to go with it 👍

Thanks 🎉

@kentcdodds I had to do the same thing in my original solution. I don't see much benefit from LoaderFunction other than typing the arguments (since the return type is just any or Promise<any>) but LoaderArgs does the same thing.🤷

Yep. And now I get to use function declarations 🎉

Should this approach provide an easy type for data that's passed to the Meta function?

Having a hard time narrowing this type

type LoaderData = Awaited<ReturnType<typeof loader>> // Response & { json(): Promise<{ name: string }> }

@theMosaad I think you're looking for this:

type LoaderData = UseDataFunctionReturn<typeof loader>

:)

But yeah, I think we could probably do something to help with this in the MetaFunction type 🤔

Hi! Is there any official documentation for this or only in this PR? Thanks!

@kubahasek not yet, but it's being worked on. All the docs/examples/stacks should get an update soon.
For now, this is documented in this CHANGELOG.md file

@theMosaad I think you're looking for this:

type LoaderData = UseDataFunctionReturn<typeof loader>

:)

But yeah, I think we could probably do something to help with this in the MetaFunction type 🤔

@kentcdodds This is working great for me! Is it normal/expected that I should import { UseDataFunctionReturn } from '@remix-run/react/dist/components'? It seems like the type is only available via dist because @remix-run/react/index.tsx doesn't re-export it from the components module.

Oh, we should definitely export that. PR welcome! 😅

@kentcdodds PR to export UseDataFunctionReturn type: #3764