TS: Improvement for LoaderFunction

[akomm]

Change:

export interface LoaderFunction {
    (args: DataFunctionArgs): Promise<Response> | Response | Promise<AppData> | AppData;
}

To:

export interface LoaderFunction<T = Response | AppData> {
    (args: DataFunctionArgs): T | Promise<T>;
}

1. Reduction: Promise<A> | Promise<B> is equivalent to Promise<A | B>
2. Backward-compatible default for T (= Response | AppData)
3. Benefit 1: removed redundancy in typing
4. Benefit 2:

// Instead of:
type LoaderData = {
  products: Product[]
}

// for async need to wrap in Promise
export const loader: LoaderFunction = async (): Promise<LoaderData> => {
  return {
    products: await productsQuery()
  }
}

export default function ProductsRoute() {
  const {products} = useLoaderData<LoaderData>()

  return <ProductTable products={products} />
}

// you can now instead:
type LoaderData = {
  products: Product[]
}

// no need to wrap in Promise<...> for async
export const loader: LoaderFunction<LoaderData> = async () => {
  return {
    products: await productsQuery()
  }
}

export default function ProductsRoute() {
  const {products} = useLoaderData<LoaderData>()

  return <ProductTable products={products} />
}

Side-note:
Currently, AppData = any. Using any in an union, will always reduce to any. So actually, Promise<AppData> will reduce to any due to AppData in the union. You won't ever see Promise<AppData>. So given current typing, even without the above improvements, you can remove Promise<AppData>.

[akomm]

Some demo:
TS playground

[hollandThomas]

Somewhat related: microsoft/TypeScript: Type annotation for all exports in the module

I wonder if it would be possible to have e.g.

// Internal Remix code
interface EntryRouteModule<LoaderData = Response | AppData> {
  CatchBoundary?: CatchBoundaryComponent;
  ErrorBoundary?: ErrorBoundaryComponent;
  default?: RouteComponent;
  handle?: RouteHandle;
  links?: LinksFunction;
  meta?: MetaFunction | HtmlMetaDescriptor;
  action?: ActionFunction;
  headers?: HeadersFunction | { [name: string]: string };
  loader?: LoaderFunction<LoaderData>;
}

Then, in a route

// routes/products.tsx
import type { EntryRouteModule } from 'Remix'
import { useLoaderData } from 'Remix';

export implements EntryRouteModule<Product[]>;

// this is typed correctly because of the line above
export const loader = async () => {
  return {
    products: await productsQuery()
  }

export default function ProductsRoute() {
  // Have TypeScript figure this out automatically somehow:
  // useLoaderData now knows that `LoaderData === Product[]`. There is no need to `useLoaderData<Product[]>`
  const {products} = useLoaderData()

  return <ProductTable products={products} />
}

[akomm]

Typescript can't, since the proposal for export implements would be in your route file, not remix. You never import your route file yourself, hence TS will not check anything.

I kind of like the simplicity of how it works with remix now. I also used trpc (not in remix...) with its inferred approach, but it has also its downsides. Sure, you can make an error in remix, by typing differently. But since its co-located I'd rated it as a trivial problem that you might have the types not properly set in two places in the same file. Especially when they just one above the other. I have nothing against making it even better, but also need to consider the price of adding complexity.

[hollandThomas]

Yeah, this would require to extend the export implements proposal. Something like "A module that implements named exports may infer the types of any imports that originate from the same module that provides the named export interface".

I agree, it's kind of trivial to set the same type twice. But a possible source of bugs nevertheless. In my experience, things like this happen and can slip through easily.

Anyway, this idea is more in the spirit of what Rich Harris said
microsoft/TypeScript#38511 (comment)

Nevertheless, it's very useful in my experience to understand the rough shape of feature B (and any other dependent features you have in mind) before designing feature A

[hollandThomas]

I proposed the idea in the original TypeScript issue. That should be a better place for that
microsoft/TypeScript#38511 (comment)

[akomm]

The TS proposal is great. It would do more than just what this proposal does. It just requires TS changes, so I stick to the simple one first. I also missunderstood something in the proposal (realized it due to some weirdness that resulted from that). The confusion was because of the inversion of dependency: remix loads your routes, but since its loaded dynamically, it can not be statically type-checked. But that is what you additionally want? Anyway, as you'v said its probably something for the TS topic. :)

The proposal is a contract from the "module" file that it implements a certain interface. It does much more than just preventing you typing Products twice. It would make migrations simpler, when major changes are not BC. You would see in your route modules that API has changed. You could have auto completion for exports in IDE. And yes, generics could be propagated to multiple exported types so that they are guaranteed to be sync.

[kvnwolf]

Excellent proposal. This could also work to infer the data returned by a loader. For example:

const loader: LoaderFunction = () => 'foo'
const data = useLoaderData<InferLoaderData<typeof loader>>

[g4rcez]

I post issue #1703. We can use two generics instead one.
The first generics tell about request data and the second is return type of action/loader

[na2hiro]

I believe this would help ensuring type safety between server and client better!

However, the proposed type has inconvenience when a loader also returns Responses in addition to objects. Modifying the given example:

type LoaderData = {
  products: Product[]
}

// Type Error for `loader` because LoaderData != Response.
// You can instead write `LoaderFunction<LoaderData | Response>`, but it's kind of redundant.
export const loader: LoaderFunction<LoaderData> = async ({params}) => {
  // Conditionally return with specific header
  if (params.archived) {
    return json(await archivedProducts(), {headers: {'Cache-Control': 'public, max-age=31536000, immutable'}})
  }
  return {
    products: await productsQuery()
  }
}

export default function ProductsRoute() {
  const {products} = useLoaderData<LoaderData>()

  return <ProductTable products={products} />
}

I think LoaderFunction should always allow return type of Response. I propose this instead, which allow us to only narrow AppData (= any) to given generic type T.:

-export interface LoaderFunction {
-    (args: DataFunctionArgs): Promise<Response> | Response | Promise<AppData> | AppData;
-}
+interface BetterLoaderFunction<T = AppData> {
+    (args: DataFunctionArgs): T | Response | Promise<T | Response>;
+}

[na2hiro]

I believe ActionFunction could be extended in the similar way and we'll get the same benefits of type safety:

export interface ActionFunction<T = AppData> {
    (args: DataFunctionArgs): T | Response | Promise<T | Response>;
}

[akomm]

@na2hiro
What you want is covered and considered already by the proposal:

export const loader: LoaderFunction<LoaderData | Response>

Technically its always a response what you return. However, you want the data returned and used by client to be as type-safe as possible. Forcing Response into it, removes a slice of the safety, because now you can return an arbitrary response much easier in cases, where you only want to return data. Now the type-system will not notify you, if you accidentally return a response, where you wanted to return specific data. If you have the case of returning a response as well (conditional redirect for example), you need to provide this possibility as type-param.

The point of this proposal is just to eliminate the need of typing Promise explicitly and also remove Promise<any>, because its effectivelly eliminated by TS in union with any. Union any | Promise<any> will be reduced to any by typescript. You won't ever see Promise-specific API by this typing.

[na2hiro]

(context: ^ is a reply to #1401 (comment))

Yeah, I know Response could be also returned like that, as I mentioned in the comment in my snippet.

I think I have slightly different view point of Response from yours. If Response should be something exceptional or preferred not to be used often, then I agree your typing makes more sense. However, Response is required for many use cases like setting different cache headers depending on loader data, which is recommended by Remix for performance optimizations, specifying HTTP status code, etc. I think it's weird if devs have to treat Response as something exceptional and have to widen the type in the generics.

I think this what-if could hopefully describe my view point in a different way: What if Response becomes generics which annotates that the body is a JSON of the given type. (kinda nice right?) Should we write LoaderFunction<LoaderData | Response<LoaderData>>, or LoaderFunction<LoaderData>? I think people would go for the latter, which could mean that LoaderFunction<LoaderData> is more natural than LoaderFunction<LoaderData | Response> today.

I see it's unfortunate that we cannot type data in Response, but we shouldn't alienate it in typing because of that. I'm curious about Remix team's opinion too!

[akomm]

@na2hiro
Response generic does not make any sense, since it represent any possible response status & data, which includes, but is not limited to json. This is the reason, why the json helper function exists, which is in fact generic. With json you can also set the init, like you can with new Response. What you basically want already exists and for any other type of response, you can add another helper function like json.

The problem I currently see, I'm open to be convince the other way by argument, that you want something to not have to write it explicitly, even though it removes type-safety level. I think one should only remove the need to be be explicit, if the explicity has no reason to exist and is just noise. Otherwise, you can aswell also entirely opt-out of the need using any.

[na2hiro]

ok, json<>() can be used to make sure the type passed to generics of json. However, if we use json() once in a loader, we still need to add Response to your new LoaderFunction generics, right? I think that's what's not the right way to go.

I have an idea for type safety around loaders outside the LoaderFunction itself. Will make another discussion probably.

[sergiodxa]

Here’s a comment from Ryan explaining why actions and loaders can’t be generics #1254 (comment)

[g4rcez]

Thank you @sergiodxa. I think we can build a generics types for loader function in some package, like remix-types. And the user of package assumes the responsability for "type safe". Maybe this soluction is not the best because we have to rewrite Response native API. Any ideias for this or we can just give up for generics in loader/useLoaderData?

[hollandThomas]

Take a look at this
https://github.com/joeflateau/remix-superloader

[na2hiro]

We've got official improvements for the typing of loaders!

#3276 https://github.com/remix-run/remix/releases/tag/remix%401.6.5