Copy Remix decision docs over

decisions/0001-use-npm-to-manage-npm-dependencies-for-deno-projects.md

@@ -0,0 +1,113 @@
+# Use `npm` to manage NPM dependencies for Deno projects
+
+Date: 2022-05-10
+
+Status: accepted
+
+## Context
+
+Deno has three ways to manage dependencies:
+
+1. Inlined URL imports: `import {...} from "https://deno.land/x/blah"`
+2. [deps.ts](https://deno.land/manual/examples/manage_dependencies)
+3. [Import maps](https://deno.land/manual/linking_to_external_code/import_maps)
+
+Additionally, NPM packages can be accessed as Deno modules via [Deno-friendly CDNs](https://deno.land/manual/node/cdns#deno-friendly-cdns) like https://esm.sh.
+
+Remix has some requirements around dependencies:
+
+- Remix treeshakes dependencies that are free of side-effects.
+- Remix sets the environment (dev/prod/test) across all code, including dependencies, at runtime via the `NODE_ENV` environment variable.
+- Remix depends on some NPM packages that should be specified as peer dependencies (notably, `react` and `react-dom`).
+
+### Treeshaking
+
+To optimize bundle size, Remix [treeshakes](https://esbuild.github.io/api/#tree-shaking) your app's code and dependencies.
+This also helps to separate browser code and server code.
+
+Under the hood, the Remix compiler uses [esbuild](https://esbuild.github.io).
+Like other bundlers, `esbuild` uses [`sideEffects` in `package.json` to determine when it is safe to eliminate unused imports](https://esbuild.github.io/api/#conditionally-injecting-a-file).
+
+Unfortunately, URL imports do not have a standard mechanism for marking packages as side-effect free.
+
+### Setting dev/prod/test environment
+
+Deno-friendly CDNs set the environment via a query parameter (e.g. `?dev`), not via an environment variable.
+That means changing environment requires changing the URL import in the source code.
+While you could use multiple import maps (`dev.json`, `prod.json`, etc...) to workaround this, import maps have other limitations:
+
+- standard tooling for managing import maps is not available
+- import maps are not composeable, so any dependencies that use import maps must be manually accounted for
+
+### Specifying peer dependencies
+
+Even if import maps were perfected, CDNs compile each dependency in isolation.
+That means that specifying peer dependencies becomes tedious and error-prone as the user needs to:
+
+- determine which dependencies themselves depend on `react` (or other similar peer dependency), even if indirectly.
+- manually figure out which `react` version works across _all_ of these dependencies
+- set that version for `react` as a query parameter in _all_ of the URLs for the identified dependencies
+
+If any dependencies change (added, removed, version change),
+the user must repeat all of these steps again.
+
+## Decision
+
+### Use `npm` to manage NPM dependencies for Deno
+
+Do not use Deno-friendly CDNs for NPM dependencies in Remix projects using Deno.
+
+Use `npm` and `node_modules/` to manage NPM dependencies like `react` for Remix projects, even when using Deno with Remix.
+
+Deno module dependencies (e.g. from `https://deno.land`) can still be managed via URL imports.
+
+### Allow URL imports
+
+Remix will preserve any URL imports in the built bundles as external dependencies,
+letting your browser runtime and server runtime handle them accordingly.
+That means that you may:
+
+- use URL imports for the browser
+- use URL imports for the server, if your server runtime supports it
+
+For example, Node will throw errors for URL imports, while Deno will resolve URL imports as normal.
+
+### Do not support import maps
+
+Remix will not yet support import maps.
+
+## Consequences
+
+- URL imports will not be treeshaken.
+- Users can specify environment via the `NODE_ENV` environment variable at runtime.
+- Users won't have to do error-prone, manual dependency resolution.
+
+### VS Code type hints
+
+Users may configure an import map for the [Deno extension for VS Code](denoland.vscode-deno) to enable type hints for NPM-managed dependencies within their Deno editor:
+
+`.vscode/resolve_npm_imports_in_deno.json`
+
+```json
+{
+  "// This import map is used solely for the denoland.vscode-deno extension.": "",
+  "// Remix does not support import maps.": "",
+  "// Dependency management is done through `npm` and `node_modules/` instead.": "",
+  "// Deno-only dependencies may be imported via URL imports (without using import maps).": "",
+
+  "imports": {
+    "react": "https://esm.sh/react@18.0.0",
+    "react-dom": "https://esm.sh/react-dom@18.0.0",
+    "react-dom/server": "https://esm.sh/react-dom@18.0.0/server"
+  }
+}
+```
+
+`.vscode/settings.json`
+
+```json
+{
+  "deno.enable": true,
+  "deno.importMap": "./.vscode/resolve_npm_imports_in_deno.json"
+}
+```

decisions/0002-do-not-clone-request.md

@@ -0,0 +1,19 @@
+# Do not clone request
+
+Date: 2022-05-13
+
+Status: accepted
+
+## Context
+
+To allow multiple loaders / actions to read the body of a request, we have been cloning the request before forwarding it to user-code. This is not the best thing to do as some runtimes will begin buffering the body to allow for multiple consumers. It also goes against "the platform" that states a request body should only be consumed once.
+
+## Decision
+
+Do not clone requests before they are passed to user-code (actions, handleDocumentRequest, handleDataRequest), and remove body from request passed to loaders. Loaders should be thought of as a "GET" / "HEAD" request handler. These request methods are not allowed to have a body, therefore you should not be reading it in your Remix loader function.
+
+## Consequences
+
+Loaders always receive a null body for the request.
+
+If you are reading the request body in both an action and handleDocumentRequest or handleDataRequest this will now fail as the body will have already been read. If you wish to continue reading the request body in multiple places for a single request against recommendations, consider using `.clone()` before reading it; just know this comes with tradeoffs.

decisions/0003-infer-types-for-useloaderdata-and-useactiondata-from-loader-and-action-via-generics.md

@@ -0,0 +1,230 @@
+# Infer types for `useLoaderData` and `useActionData` from `loader` and `action` via generics
+
+Date: 2022-07-11
+
+Status: accepted
+
+## Context
+
+Goal: End-to-end type safety for `useLoaderData` and `useActionData` with great Developer Experience (DX)
+
+Related discussions:
+
+- [remix-run/remix#1254](https://github.com/remix-run/remix/pull/1254)
+- [remix-run/remix#3276](https://github.com/remix-run/remix/pull/3276)
+
+---
+
+In Remix v1.6.4, types for both `useLoaderData` and `useActionData` are parameterized with a generic:
+
+```tsx
+type MyLoaderData = {
+  /* ... */
+};
+type MyActionData = {
+  /* ... */
+};
+
+export default function Route() {
+  const loaderData = useLoaderData<MyLoaderData>();
+  const actionData = useActionData<MyActionData>();
+  return <div>{/* ... */}</div>;
+}
+```
+
+For end-to-end type safety, it is then the user's responsability to make sure that `loader` and `action` also use the same type in the `json` generic:
+
+```ts
+export const loader: LoaderFunction = () => {
+  return json<MyLoaderData>({
+    /* ... */
+  });
+};
+
+export const action: ActionFunction = () => {
+  return json<MyActionData>({
+    /* ... */
+  });
+};
+```
+
+### Diving into `useLoaderData`'s and `useActionData`'s generics
+
+Tracing through the `@remix-run/react` source code (v1.6.4), you'll find that `useLoaderData` returns an `any` type that is implicitly type cast to whatever type gets passed into the `useLoaderData` generic:
+
+```ts
+// https://github.com/remix-run/remix/blob/v1.6.4/packages/remix-react/components.tsx#L1370
+export function useLoaderData<T = AppData>(): T {
+  return useRemixRouteContext().data; //
+}
+
+// https://github.com/remix-run/remix/blob/v1.6.4/packages/remix-react/components.tsx#L73
+function useRemixRouteContext(): RemixRouteContextType {
+  /* ... */
+}
+
+// https://github.com/remix-run/remix/blob/v1.6.4/packages/remix-react/components.tsx#L56
+interface RemixRouteContextType {
+  data: AppData;
+  id: string;
+}
+
+// https://github.com/remix-run/remix/blob/v1.6.4/packages/remix-react/data.ts#L4
+export type AppData = any;
+```
+
+Boiling this down, the code looks like:
+
+```ts
+let data: any;
+
+// somewhere else, `loader` gets called an sets `data` to some value
+
+function useLoaderData<T>(): T {
+  return data; // <-- Typescript casts this `any` to `T`
+}
+```
+
+`useLoaderData` isn't basing its return type on how `data` was set (i.e. the return value of `loader`) nor is it validating the data.
+It's just blindly casting `data` to whatever the user passed in for the generic `T`.
+
+### Issues with current approach
+
+The developer experience is subpar.
+Users are required to write redundant code for the data types that could have been inferred from the arguments to `json`.
+Changes to the data shape require changing _both_ the declared `type` or `interface` as well as the argument to `json`.
+
+Additionally, the current approach encourages users to pass the same type to `json` with the `loader` and to `useLoaderData`, but **this is a footgun**!
+`json` can accept data types like `Date` that are JSON serializable, but `useLoaderData` will return the _serialized_ type:
+
+```ts
+type MyLoaderData = {
+  birthday: Date;
+};
+
+export const loader: LoaderFunction = () => {
+  return json<MyLoaderData>({ birthday: new Date("February 15, 1992") });
+};
+
+export default function Route() {
+  const { birthday } = useLoaderData<MyLoaderData>();
+  // ^ `useLoaderData` tricks Typescript into thinking this is a `Date`, when in fact its a `string`!
+}
+```
+
+Again, the same goes for `useActionData`.
+
+### Solution criteria
+
+- Return type of `useLoaderData` and `useActionData` should somehow be inferred from `loader` and `action`, not blindly type cast
+- Return type of `loader` and `action` should be inferred
+  - Necessarily, return type of `json` should be inferred from its input
+- No module side-effects (so higher-order functions like `makeLoader` is definitely a no).
+- `json` should allow everything that `JSON.stringify` allows.
+- `json` should allow only what `JSON.stringify` allows.
+- `useLoaderData` should not return anything that `JSON.parse` can't return.
+
+### Key insight: `loader` and `action` are an _implicit_ inputs
+
+While there's been interest in inferring the types for `useLoaderData` based on `loader`, there was [hesitance to use a Typescript generic to do so](https://github.com/remix-run/remix/pull/3276#issuecomment-1164764821).
+Typescript generics are apt for specifying or inferring types for _inputs_, not for blindly type casting output types.
+
+A key factor in the decision was identifying that `loader` and `action` are _implicit_ inputs of `useLoaderData` and `useActionData`.
+
+In other words, if `loader` and `useLoaderData` were guaranteed to run in the same process (and not cross the network), then we could write `useLoaderData(loader)`, specifying `loader` as an explicit input for `useLoaderData`.
+
+```ts
+// _conceptually_ `loader` is an input for `useLoaderData`
+function useLoaderData<Loader extends LoaderFunction>(loader: Loader) {
+  /*...*/
+}
+```
+
+Though `loader` and `useLoaderData` exist together in the same file at development-time, `loader` does not exist at runtime in the browser.
+Without the `loader` argument to infer types from, `useLoaderData` needs a way to learn about `loader`'s type at compile-time.
+
+Additionally, `loader` and `useLoaderData` are both managed by Remix across the network.
+While its true that Remix doesn't "own" the network in the strictest sense, having `useLoaderData` return data that does not correspond to its `loader` is an exceedingly rare edge-case.
+
+Same goes for `useActionData`.
+
+---
+
+A similar case is how [Prisma](https://www.prisma.io/) infers types from database schemas available at runtime, even though there are (exceedingly rare) edge-cases where that database schema _could_ be mutated after compile-time but before run-time.
+
+## Decision
+
+Explicitly provide type of the implicit `loader` input for `useLoaderData` and then infer the return type for `useLoaderData`.
+Do the same for `action` and `useActionData`.
+
+```ts
+export const loader = async (args: LoaderArgs) => {
+  // ...
+  return json(/*...*/);
+};
+
+export default function Route() {
+  const data = useLoaderData<typeof loader>();
+  // ...
+}
+```
+
+Additionally, the inferred return type for `useLoaderData` will only include serializable (JSON) types.
+
+### Return `unknown` when generic is omitted
+
+Omitting the generic for `useLoaderData` or `useActionData` results in `any` being returned.
+This hides potential type errors from the user.
+Instead, we'll change the return type to `unknown`.
+
+```ts
+type MyLoaderData = {
+  /*...*/
+};
+
+export default function Route() {
+  const data = useLoaderData();
+  // ^? unknown
+}
+```
+
+Note: Since this would be a breaking change, changing the return type to `unknown` will be slated for v2.
+
+### Deprecate non-inferred types via generics
+
+Passing in a non-inferred type for `useLoaderData` is hiding an unsafe type cast.
+Using the `useLoaderData` in this way will be deprecated in favor of an explicit type cast that clearly communicates the assumptions being made:
+
+```ts
+type MyLoaderData = {
+  /*...*/
+};
+
+export default function Route() {
+  const dataGeneric = useLoaderData<MyLoaderData>(); // <-- will be deprecated
+  const dataCast = useLoaderData() as MyLoaderData; // <- use this instead
+}
+```
+
+## Consequences
+
+- Users can continue to provide non-inferred types by type casting the result of `useLoaderData` or `useActionData`
+- Users can opt-in to inferred types by using `typeof loader` or `typeof action` at the generic for `useLoaderData` or `useActionData`.
+- Return types for `loader` and `action` will be the sources-of-truth for the types inferred for `useLoaderData` and `useActionData`.
+- Users do not need to write redundant code to align types across the network
+- Return type of `useLoaderData` and `useActionData` will correspond to the JSON _serialized_ types from `json` calls in `loader` and `action`, eliminating a class of errors.
+- `LoaderFunction` and `ActionFunction` should not be used when opting into type inference as they override the inferred return types.[^1]
+
+🚨 Users who opt-in to inferred types **MUST** return a `TypedResponse` from `json` and **MUST NOT** return a bare object:
+
+```ts
+const loader = () => {
+  // NO
+  return { hello: "world" };
+
+  // YES
+  return json({ hello: "world" });
+};
+```
+
+[^1]: The proposed `satisfies` operator for Typescript would let `LoaderFunction` and `ActionFunction` enforce function types while preserving the narrower inferred return type: https://github.com/microsoft/TypeScript/issues/47920