remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.9k stars 2.52k forks source link

[Feature]: end-to-end type safety #1330

Closed curwen closed 2 years ago

curwen commented 2 years ago

What is the new or updated feature that you are suggesting?

End-to-end type safety by inferring the return type of useDataLoader from the loader. Possibly achieved by either using superJSON to actually encode/decode the correct data structure, or inferring the correct de-serialised JSON response i.e Date->string.

Why should this feature be included?

Type annotations, type arguments, and type guards on an Any can potentially be incorrect / out of sync, providing a false sense of type safety. By inferring the response type from the loader we can mitigate that risk and gain true end-to-end type safety.

Using a generic type argument for useLoaderData such as ReturnType<typeof loader>> is possibly the 'safest' generic type argument for useLoaderData, except due to the data being serialised by JSON it's vulnerable to the following issue:

export const loader = async () => {
  const user = {name: 'bob', dateOfBirth: new Date()};
  const data = {
    user
  };
  return data;
};

export default function MyComponent() {
  const { user } = useLoaderData<Awaited<ReturnType<typeof loader>>>();
  const seconds = user.dateOfBirth.getSeconds(); // Runtime error, dateOfBirth is a string here.
  return <div>Hello</div>;
}

Example of potential TS compiler solution:

export const loader = async () => {
  const user = {name: 'bob', dateOfBirth: new Date()};
  const data = {
    user
  };
  return data;
};

export default function MyComponent() {
  const { user } = useLoaderData();
  const seconds = user.dateOfBirth.getSeconds(); // TS compiler error, dateOfBirth is a string!
  return <div>Hello</div>;
}

Example of potential superJSON solution:

export const loader = async () => {
  const user = {name: 'bob', dateOfBirth: new Date()};
  const data = {
    user
  };
  return data;
};

export default function MyComponent() {
  const { user } = useLoaderData();
  const seconds = user.dateOfBirth.getSeconds(); // no issue as superJSON reconstructed the Date object for us.
  return <div>Hello</div>;
}

I'd be interested to hear other people's thoughts on this and any potential technical issues or solutions.

kiliman commented 2 years ago

Here's an example you can use to get you close to what frameworks like Blitz provide.

https://codesandbox.io/s/superjson-types-p1n7t?file=/app/routes/index.tsx

import { LoaderFunction } from "remix";
import { json, useLoaderData } from "~/utils/superjson";
import { getProject } from "~/queries/project.server";

export const loader: LoaderFunction = () => {
  return json(getProject(1));
};

export default function Index() {
  const { name, date } = useLoaderData(getProject);
  return (
    <div>
      <h1>{name}</h1>
      <p>Date is {date.toLocaleDateString()}</p>
    </div>
  );
}

The example creates wrappers for json() and useLoaderData() which utilize superJSON for serializing/deserializing the payload.

NOTE: it's the getProject() function return type that is encoded. In the call to useLoaderData() we pass a reference to this function so it can determine the return type. Since this is referenced in the component, to ensure that the function is not included in the client-side bundle, we import it from a .server file.

akomm commented 2 years ago

@kiliman if getProject is not included in client, how does the code run on client? On initial render its ssr, but when you client-navigate? useLoaderData accepts undefined? Sounds weird. I guess I'm missing some part in the idea?

kiliman commented 2 years ago

@akomm Remember that all loaders are run server-side and useLoaderData() simply gets the data that was fetched by Remix. The reference to getProject there is simply to get the return type of that function. It's not actually called there. You can see that it is called in the loader function (which again is done on the server).

kiliman commented 2 years ago

In case it's not clear, I created replacements for the standard Remix json and userLoaderData functions.

import superjson from "superjson";
import {
  json as remix_json,
  useLoaderData as remix_useLoaderData
} from "remix";

export function json<Data>(data: Data, init?: number | ResponseInit): Response {
  return remix_json(superjson.serialize(data), init);
}

export function useLoaderData<T extends (...args: any) => any>(
  fn: T
): ReturnType<T> {
  return superjson.deserialize<ReturnType<T>>(remix_useLoaderData());
}
kiliman commented 2 years ago

Looks like there is a PR to include superJSON in core Remix #662

akomm commented 2 years ago

@kiliman you just explained what I (and from comment should be clear) understand, but did not answer the question. What is getProject in the components at runtime?

kiliman commented 2 years ago

Hmmm. As I stated in my reply, getProjects in useLoaderData(getProjects) is there simply so the data is typed as the return type of getProjects. You can see it's a reference, not a function call, no (). This is simply to satisfy TypeScript. Since getProjects is defined in the .server.ts file, it'll be stubbed out to nothing. Since we never call it from the component, it won't cause any issues.

Again, this is just a hack to enable "inferred types" like the OP desired. Personally I prefer explicit types.

jmarbutt commented 2 years ago

I agree, is there a way to not infer, I am seeing if I can rework it for that.

jmarbutt commented 2 years ago

I changed the utils to this

import superjson from "superjson";
import {
  json as remix_json,
  useLoaderData as remix_useLoaderData
} from "remix";

export function json<Data>(data: Data, init?: number | ResponseInit): Response {
  return remix_json(superjson.serialize(data), init);
}

export function useLoaderData<T>() {
  return superjson.deserialize<T>(remix_useLoaderData());
}

And it seems to work

akomm commented 2 years ago

@jmarbutt yes your example makes sense, its basically wraps superjson to use, but otherwise works like default userLoaderData in terms of typing. @kiliman I still don't see (but would like) how passing an unused function just to infer the type is better, than simply passing the generics the way you can already. In both cases, you have to either write the generic in 2 places or the function name expression. In both cases, you are responsible to have them both sync.

kiliman commented 2 years ago

@akomm Yes, I agree. Much better to be explicit and use generic types.

type LoaderType = {}
return json<LoaderType>()
const data = useLoaderData<LoaderType>()

The example I made was to allow implicit typing that @curwen had requested. This is similar to how Blitz worked. So again, I was just using the function reference to get the return type, and since the function was imported from a .server file, that function was guaranteed not to be included in the client bundle. This was fine because we don't actually call the function here.

kiliman commented 2 years ago

I think your confusion may be due to lack of context. There was a big thread on Discord discussing the desire by OP to not have to use any explicit types, and only use inferred types.

Here's the link to the first message. You can follow the discussion in the thread: types over the network.

https://discord.com/channels/770287896669978684/770287896669978687/925792653671534682

curwen commented 2 years ago

The issue with the above code snippets is that they're essentially just being explicit in a different way (an argument rather than a type variable). The loader function could change and your useLoaderData hook could still be pointing to getProjects so you'd never get a type error.

I suppose the original point was to reduce boiler plate so that you don't have to write (and maintain) a reference to something that could change, and consequently break something. The second point was to ensure that should Remix manage to infer the types from the loader function, that the types are actually correct after encoding/decoding.

In the Blitz documentation they explain how they do it: 'You may be wondering how that can work since it's importing server code into your component: At build time, the direct function import is swapped out with a network call. So the query function code is never included in your client code.'

Blitz then use superJSON to handle the type re-construction, which we could potentially use (through a different hook), or potentially use a type such as

type ToJSON<T> = T extends Array<infer i> ? Array<ToJSON<i>> : T extends { toJSON: (...args: any) => infer R } ? R : T extends object ? { [K in keyof T]: K extends string | number ? ToJSON<T[K]> : never } : T;

...which doesn't reconstruct the type, but at least is a bit more accurate?

kiliman commented 2 years ago

No offense, but you act like Blitz does something magical.

How is this (from Blitz tutorial) any different than what I suggested?

Note the getQuestions reference in usePaginatedQuery()... same thing as getProjects. Again, it's only used so the hook can infer the return type.

As for superJSON, did you actually look at the code I wrote? It uses superJSON to manage the types across the loader (https://github.com/remix-run/remix/issues/1330#issuecomment-1007618984).

export const QuestionsList = () => {
  const router = useRouter()
  const page = Number(router.query.page) || 0
  const [{ questions, hasMore }, { isPreviousData }] = usePaginatedQuery(
    getQuestions,
    {
      orderBy: { id: "asc" },
      skip: ITEMS_PER_PAGE * page,
      take: ITEMS_PER_PAGE,
    }
  )
  ...

https://blitzjs.com/docs/tutorial#question-pages

curwen commented 2 years ago

How is this (from Blitz tutorial) any different than what I suggested?

The difference here is that there's nothing to keep in sync. If the return type of getQuestions changes, we'll know about it (type error). In your example though, if the return type of the loader changes, we won't get an error, since useLoaderData is still referencing getProjects which is now out-of-date/stale.

As for superJSON, did you actually look at the code I wrote? It uses superJSON to manage the types across the loader (#1330 (comment)).

Yes, sorry I wasn't clear there, when I said we could potentially use a different hook, I was referring to your implementation there (which I like). I was also just pointing out that we don't necessarily need to use superJSON / reconstruct the type and could instead use a generic JSON type that recursively aliases to the object's own Stringification type.

chaance commented 2 years ago

Much of this is discussed at length in https://github.com/remix-run/remix/pull/1254#issuecomment-1034475233. Closing as a duplicate.

We don't intend to support returning non-serializable data from loaders and actions, but this can be accomplished in user-land. @donavon created a tool specifically for that use-case: https://www.npmjs.com/package/superjson-remix