diff --git a/decisions/0001-use-npm-to-manage-npm-dependencies-for-deno-projects.md b/decisions/0001-use-npm-to-manage-npm-dependencies-for-deno-projects.md index 35a6ec0a363..377ee62ebb1 100644 --- a/decisions/0001-use-npm-to-manage-npm-dependencies-for-deno-projects.md +++ b/decisions/0001-use-npm-to-manage-npm-dependencies-for-deno-projects.md @@ -16,19 +16,19 @@ Additionally, NPM packages can be accessed as Deno modules via [Deno-friendly CD Remix has some requirements around dependencies: -- Remix treeshakes dependencies that are free of side-effects. +- Remix tree shakes 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 +### Tree shaking -To optimize bundle size, Remix [treeshakes](https://esbuild.github.io/api/#tree-shaking) your app's code and dependencies. +To optimize bundle size, Remix [tree shakes](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). +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. +Unfortunately, URL imports do not have a standard mechanism for marking packages as side-effect-free. ### Setting dev/prod/test environment @@ -37,7 +37,7 @@ That means changing environment requires changing the URL import in the source c 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 +- import maps are not composable, so any dependencies that use import maps must be manually accounted for ### Specifying peer dependencies @@ -46,10 +46,9 @@ That means that specifying peer dependencies becomes tedious and error-prone as - 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 +- set that version for `react` as a query parameter in _all_ the URLs for the identified dependencies -If any dependencies change (added, removed, version change), -the user must repeat all of these steps again. +If any dependencies change (added, removed, version change), the user must repeat all of these steps. ## Decision @@ -59,7 +58,7 @@ 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. +Deno module dependencies (e.g., from `https://deno.land`) can still be managed via URL imports. ### Allow URL imports @@ -68,7 +67,7 @@ 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 +- 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. @@ -78,7 +77,7 @@ Remix will not yet support import maps. ## Consequences -- URL imports will not be treeshaken. +- URL imports will not be tree shaken. - Users can specify environment via the `NODE_ENV` environment variable at runtime. - Users won't have to do error-prone, manual dependency resolution. diff --git a/decisions/0002-do-not-clone-request.md b/decisions/0002-do-not-clone-request.md index 30f599f7f0f..e96da400992 100644 --- a/decisions/0002-do-not-clone-request.md +++ b/decisions/0002-do-not-clone-request.md @@ -1,4 +1,4 @@ -# Do not clone request +# Do not clone a request Date: 2022-05-13 @@ -10,10 +10,10 @@ To allow multiple loaders / actions to read the body of a request, we have been ## 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. +Do not clone requests before they are passed to user-code (actions, handleDocumentRequest, handleDataRequest), and remove the body from the `request` passed to `loader`s. 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. +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; know this comes with tradeoffs. diff --git a/decisions/0003-infer-types-for-useloaderdata-and-useactiondata-from-loader-and-action-via-generics.md b/decisions/0003-infer-types-for-useloaderdata-and-useactiondata-from-loader-and-action-via-generics.md index 1959f161835..d4c6b12884b 100644 --- a/decisions/0003-infer-types-for-useloaderdata-and-useactiondata-from-loader-and-action-via-generics.md +++ b/decisions/0003-infer-types-for-useloaderdata-and-useactiondata-from-loader-and-action-via-generics.md @@ -32,7 +32,7 @@ export default function Route() { } ``` -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: +For end-to-end type safety, it is then the user's responsibility to make sure that `loader` and `action` also use the same type in the `json` generic: ```ts export const loader: LoaderFunction = () => { @@ -85,7 +85,7 @@ function useLoaderData(): 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. +`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 @@ -119,15 +119,15 @@ Again, the same goes for `useActionData`. - 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). +- 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 +### Key insight: `loader` and `action` are _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. +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`. @@ -146,7 +146,7 @@ Without the `loader` argument to infer types from, `useLoaderData` needs a way t 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`. +The same goes for `useActionData`. --- @@ -154,7 +154,7 @@ A similar case is how [Prisma](https://www.prisma.io/) infers types from databas ## Decision -Explicitly provide type of the implicit `loader` input for `useLoaderData` and then infer the return type for `useLoaderData`. +Explicitly provide a type of the implicit `loader` input for `useLoaderData` and then infer the return type for `useLoaderData`. Do the same for `action` and `useActionData`. ```ts @@ -209,13 +209,13 @@ export default function Route() { ## 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`. +- 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: +🚨 Users who opt in to inferred types **MUST** return a `TypedResponse` from `json` and **MUST NOT** return a bare object: ```ts const loader = () => { @@ -227,4 +227,4 @@ const loader = () => { }; ``` -[^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 +[^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 diff --git a/decisions/0004-streaming-apis.md b/decisions/0004-streaming-apis.md index 5677a48e362..d27173588ab 100644 --- a/decisions/0004-streaming-apis.md +++ b/decisions/0004-streaming-apis.md @@ -12,11 +12,11 @@ Status: accepted Remix aims to provide first-class support for React 18's streaming capabilities. Throughout the development process we went through many iterations and naming schemes around the APIs we plan to build into Remix to support streaming, so this document aims to lay out the final names we chose and the reasons behind it. -It's also worth nothing that even in a single-page-application without SSR-streaming, the same concepts still apply so these decisions were made with React Router 6.4.0 in mind as well - which will support the same Data APIs from Remix. +It's also worth nothing that even in a single-page-application without SSR streaming, the same concepts still apply, so these decisions were made with React Router 6.4.0 in mind as well — which will support the same Data APIs from Remix. ## Decision -Streaming in Remix can be thought of as having 3 touch points with corresponding APIs: +Streaming in Remix can be thought of as having three touchpoints with corresponding APIs: 1. _Initiating_ a streamed response in your `loader` can be done by returning a `defer(object)` call from your `loader` in which some of the keys on `object` are `Promise` instances 2. _Accessing_ a streamed response from `useLoaderData` @@ -32,7 +32,7 @@ In the spirit of `#useThePlatform` we've chosen to leverage the `Promise` API to ### Initiating -In order to initiate a streamed response in your `loader`, you can use the `defer()` utility which accepts a JSON object with `Promise` values from your `loader`. +To initiate a streamed response in your `loader`, you can use the `defer()` utility which accepts a JSON object with `Promise` values from your `loader`. ```tsx export async function loader() { @@ -83,8 +83,8 @@ function exampleLoader3() { Other considered API names:
@@ -102,9 +102,9 @@ function Component() { ### Rendering -In order to render your `Promise` values from `useLoaderData()`, Remix provides a new `` component which handles rendering the resolved value, or propagating the rejected value through an `errorElement` or further upwards to the Route-level error boundaries. In order to access the resolved or rejected values, there are two new hooks that only work in the context of an `` component - `useAsyncValue()` and `useAsyncError()`. +To render your `Promise` values from `useLoaderData()`, Remix provides a new `` component which handles rendering the resolved value, or propagating the rejected value through an `errorElement` or further upwards to the Route-level error boundaries. To access the resolved or rejected values, there are two new hooks that only work in the context of an `` component - `useAsyncValue()` and `useAsyncError()`. -This examples shows the full set of render-time APIs: +This example shows the full set of render-time APIs: ```tsx function Component() { @@ -132,7 +132,7 @@ function MyError() { Note that `useAsyncValue` and `useAsyncError` only work in the context of an `` component. -The `` name comes from the fact that for these lazily-rendered promises, we're not `await`-ing the promise in our loader, so instead we need to `` the promise in our render function and provide a fallback UI. The `resolve` prop is intended to mimic how you'd await a resolved value in plain Javascript: +The `` name comes from the fact that for these lazily rendered promises, we're not `await`-ing the promise in our loader, so instead we need to `` the promise in our render function and provide a fallback UI. The `resolve` prop is intended to mimic how you'd await a resolved value in plain JavaScript: ```tsx // This JSX: @@ -181,12 +181,12 @@ If you do not provide an `errorElement`, then promise rejections will bubble up

We originally implemented this as a <Deferred value={promise} fallback={<Loader />} errorElement={<MyError/>} />, but eventually we chose to remove the built-in <Suspense> boundary for better composability and eventual use with <SuspenseList>. Once that was removed, and we were only using a Promise it made sense to move to a generic <Await> component that could be used with any promise, not just those coming from defer() in a loader

-

We also considered various alternatives for the hook names - most notably `useResolvedValue`/`useRejectedValue`. However, these were a bit too tightly coupled to the `Promise` nomenclature. Remember, `Await` supports non-Promise values as well as render-errors, so it would be confusing if `useResolvedValue` was handing you a non-Promise value, or if `useRejectedValue` was handing you a render error from a resolved `Promise`. `useAsyncValue`/`useAsyncError` better encompasses those scenarios as well.

+

We also considered various alternatives for the hook names — most notably `useResolvedValue`/`useRejectedValue`. However, these were a bit too tightly coupled to the `Promise` nomenclature. Remember, `Await` supports non-Promise values as well as render-errors, so it would be confusing if `useResolvedValue` was handing you a non-Promise value, or if `useRejectedValue` was handing you a render error from a resolved `Promise`. `useAsyncValue`/`useAsyncError` better encompasses those scenarios as well.

## React Router Notes -With the presence of the `` component in React Router and because the Promise's don't have to be serialized over the network - you can _technically_ just return raw Promise values on a naked object from your loader. However, this is strongly discouraged because the router will be unaware of these promises and thus won't be able to cancel them if the user navigates away prior to the promise settling. +With the presence of the `` component in React Router and because the Promise's don't have to be serialized over the network - you can _technically_ return raw Promise values on a naked object from your loader. However, this is strongly discouraged because the router will be unaware of these promises and thus won't be able to cancel them if the user navigates away before the promise settling. By forcing users to call the `defer()` utility, we ensure that the router is able to track the in-flight promises and properly cancel them. It also allows us to handle synchronous rendering of promises that resolve prior to other critical data. Without the `defer()` utility these raw Promises would need to be thrown by the `` component to the `` boundary a single time to unwrap the value, resulting in a UI flicker. diff --git a/decisions/0005-remixing-react-router.md b/decisions/0005-remixing-react-router.md index 9b24896a86c..7cf90ba13df 100644 --- a/decisions/0005-remixing-react-router.md +++ b/decisions/0005-remixing-react-router.md @@ -30,23 +30,23 @@ In [Remixing React Router][remixing router], Ryan gives an overview of the work ### Move the bulk of logic to a framework-agnostic router -Thankfully this decision was sort of already made by Ryan. Maybe a surprise to some, maybe not, the current transition manager doesn't import or reference `react` or `react-router` a single time. This is by design because the logic being handled has nothing to do with how to render the UI layer. It's all about "what route am I on?", "what route am I going to?", "how do I load data for the next route?", "how do I interrupt ongoing navigations?" etc. None of these decisions actually _care_ about how the route and its data will eventually be rendered. Instead, the router simply needs to know whether given routes _have_ components and/or error boundaries - but it doesn't need to know about them or how to render them. +Thankfully, Ryan sort of already made this decision. Maybe a surprise to some, maybe not, the current transition manager doesn't import or reference `react` or `react-router` a single time. This is by design because the logic being handled has nothing to do with how to render the UI layer. It's all about "what route am I on?", "what route am I going to?", "how do I load data for the next route?", "how do I interrupt ongoing navigations?" etc. None of these decisions actually _care_ about how the route and its data will eventually be rendered. Instead, the router simply needs to know whether given routes _have_ components and/or error boundaries — but it doesn't need to know about them or how to render them. -This is a huge advantage since it's a strict requirement in order to eventually support UI libraries other than React (namely Preact and Vue). So in the end, we have a `@remix-run/router` package with _zero_ dependencies 🔥. +This is a huge advantage since it's a strict requirement to eventually support UI libraries other than React (namely Preact and Vue). So in the end, we have a `@remix-run/router` package with _zero_ dependencies 🔥. ### Inline the `history` library into the router -`react-router@6.3` currently relies on the `history@5` package. When we first started the work, we were intending to bring `history` into the `react-router` repo and create `history@6` and it would still be a standalone package and a dependency of `@remix-run/router`. However, 3 things pushed us in a different direction and caused us to just make history a single file inside of the router, and treat it more as an implementation detail. +`react-router@6.3` currently relies on the `history@5` package. When we first started the work, we were intending to bring `history` into the `react-router` repo and create `history@6` and it would still be a standalone package and a dependency of `@remix-run/router`. However, three things pushed us in a different direction and caused us to just make history a single file inside the router and treat it more as an implementation detail. **1. History is an implementation detail in a data-aware landscape** -Now that the router is data-aware, it has to manage _both_ route data loading/mutations and the URL (or in-memory location state but for simplicity let's just talk in terms of browser-routers here). In `react-router@6.3`/`history@5` the router was purely _reactive_. It listened for `history` changes and rendered the proper UI. So if a user clicked a link, it updated `history` _and then_ informed the router "hey you should render this new location". +Now that the router is data-aware, it has to manage _both_ route data loading/mutations and the URL (or in-memory location state, but for simplicity let's just talk in terms of browser-routers here). In `react-router@6.3`/`history@5` the router was purely _reactive_. It listened for `history` changes and rendered the proper UI. So if a user clicked a link, it updated `history` _and then_ informed the router "hey you should render this new location". -This is no longer the case in a data-aware landscape. Now, when a user clicks a link, we need to first tell the router "hey the user _intends_ to go to this location." In response to that the router can initiate some data fetches but during these fetches we're still on the old page! The user is still looking at the old content, and the URL should reflect that. This fits with the "browser emulator" concept as well. If you had a non-JS landscape and a user clicked a link from `/a -> /b` and the server took 5 seconds to send back a response for `/b` - during that time the browser URL bar shows the URL and title for `/a` and a little spinner in the tab. This is exactly how we built the router, it first loads data, then it updates state and tells history to update the URL. +This is no longer the case in a data-aware landscape. Now, when a user clicks a link, we need to first tell the router "hey the user _intends_ to go to this location." In response to that, the router can initiate some data fetches, but during these fetches we're still on the old page! The user is still looking at the old content, and the URL should reflect that. This fits with the "browser emulator" concept as well. If you had a non-JS landscape and a user clicked a link from `/a -> /b` and the server took 5 seconds to send back a response for `/b` - during that time the browser URL bar shows the URL and title for `/a` and a little spinner in the tab. This is exactly how we built the router, it first loads data, then it updates the state and tells history to update the URL. -There's one caveat here when it comes to back/forward button usage. When the user navigates back/forward in the history stack we get a `popstate` event _but the URL has already been updated_. So the best we can do there is react to the new URL. This is _not_ what the browser would do in a non-JS world, but we really have no choice. +There's one caveat here when it comes to back/forward button usage. When the user navigates back/forward in the history stack, we get a `popstate` event _but the URL has already been updated_. So the best we can do there is react to the new URL. This is _not_ what the browser would do in a non-JS world, but we really have no choice. -All this being said - history is no longer a simple process of "update the URL then tell the router to re-render". History and routing are far more intertwined and behave slightly differently for PUSH/REPLACE than they do for POP navigations. For PUSH/REPLACE we go `router.navigate -> load data -> update state -> update history`, but for POP we `update history -> router.navigate -> load data -> update state`. So in PUSH, the router informs history. But in POP, history informs the router. These nuances made sense to keep the router as the public API and make history more of an internal implementation detail. +All this being said — history is no longer a simple process of "update the URL then tell the router to re-render". History and routing are far more intertwined and behave slightly differently for PUSH/REPLACE than they do for POP navigations. For PUSH/REPLACE we go `router.navigate -> load data -> update state -> update history`, but for POP we `update history -> router.navigate -> load data -> update state`. So in PUSH, the router informs history. But in POP, history informs the router. These nuances made sense to keep the router as the public API and make history more of an internal implementation detail. **2. History is being superseded via the Navigation API** @@ -54,7 +54,7 @@ With the pending [Navigation API][navigation api] in the works, there's potentia **3. Initial implementations required it** -In the first implementations we actually didn't touch the internals of `BrowserRouter` and it's non-data-aware counterparts. And due to the changes we made in `history` to not notify listeners on PUSH/REPLACE wouldn't work. So for a very short period, we actually had both history v5 and this new internal history so we _couldn't_ publish it as history v6 since you can't have multiple dependent versions. Eventually, this went away as we added a `v5Compat` flag to the new history so it could behave like v5 used to when needed. +In the first implementations we actually didn't touch the internals of `BrowserRouter` and it's non-data-aware counterparts. And due to the changes we made in `history` to not notify listeners on PUSH/REPLACE wouldn't work. So for a very short period, we actually had both history v5 and this new internal history, so we _couldn't_ publish it as history v6 since you can't have multiple dependent versions. Eventually, this went away as we added a `v5Compat` flag to the new history so it could behave like v5 used to when needed. ### `fetcher.load()` participates in revalidations @@ -79,7 +79,7 @@ We plan to export `useNavigation` in Remix and encourage folks to switch, but we **`useTransition().type` is removed** -In Remix, the `useTransition` hook returned a Transition object which had a `state` property of `"idle" | "loading" | "submitting"`. It also had a `type` property which represented sort of "sub-states" such as `"normalLoad" | "actionReload" | "loaderRedirect"` etc. In React Router we chose to get rid of the `type` field for 2 reasons: +In Remix, the `useTransition` hook returned a Transition object which had a `state` property of `"idle" | "loading" | "submitting"`. It also had a `type` property which represented sort of "substates" such as `"normalLoad" | "actionReload" | "loaderRedirect"` etc. In React Router we chose to get rid of the `type` field for two reasons: 1. In practice, we found that the _vast_ majority of the time all you needed to reference was the `state` 2. For scenarios in which you really do need to distinguish, we are pretty sure that in all cases, you can deduce the `type` from `state`, current location (`useLocation`), next location (`useNavigation().location`), and submission info (`useNavigation().formData`). @@ -101,7 +101,7 @@ Another area that changes is the `useTransition().submission` property was remov **Backwards Compatibility** -We plan to remain backwards compatible here in Remix. Very likely we'll expose the `useNavigation` hook and encourage users to move to that. And then `useTransition` will remain in a deprecated state and it will call `useNavigation` and then backfill the `type` and `submission` properties. +We plan to remain backwards compatible here in Remix. Very likely we'll expose the `useNavigation` hook and encourage users to move to that. And then `useTransition` will remain in a deprecated state, and it will call `useNavigation` and then backfill the `type` and `submission` properties. ### `
` is no longer a "submission" @@ -116,7 +116,7 @@ Functionally, these two bits of code are identical, with the only difference bei
``` -But, in Remix we were considering the latter a "submission" such that `useTransition().state === "submitting"`. In order to ensure our "navigations" reflect the browser behavior, we have changed this in the router such that GET Form submissions result in `useNavigation().state === "loading"`. +But in Remix we were considering the latter a "submission" such that `useTransition().state === "submitting"`. To ensure our "navigations" reflect the browser behavior, we have changed this in the router such that GET Form submissions result in `useNavigation().state === "loading"`. **Backwards Compatibility** @@ -132,7 +132,7 @@ Normal POST submissions that do not redirect will use a `REPLACE`: - Navigates to `/login` (history stack is `[/, /login]`) - Fills out and submits the `
` - Action does not redirect - - At this point, if the action returns a non-redirect and we were to PUSH the navigation we'd end up with a history stack of `[/, /login, /login]` and the user would be in a scenario where it would take them 2 back buttons to get "through" the login page from a subsequent route. + - At this point, if the action returns a non-redirect and we were to PUSH the navigation, we'd end up with a history stack of `[/, /login, /login]` and the user would be in a scenario where it would take them two back buttons to get "through" the login page from a subsequent route. - To avoid this, when a POST submission does not return a redirect, the router will REPLACE in the history stack, leaving us at `[/, /login]` and avoiding the duplicate history entry Normal POST submissions that _do_ redirect will use `PUSH` for the redirect: @@ -142,7 +142,7 @@ Normal POST submissions that _do_ redirect will use `PUSH` for the redirect: - Fills out and submits the `` - Action redirects to `/private` - If we treated this redirect as a REPLACE, we'd be replacing the _initial_ navigation to `/login` since we haven't yet touched history for the POST. This would leave the history stack as `[/, /private]` and we'd lose the fact that we were ever at the login page. - - Instead when an action redirects, we'll use a PUSH and in this case the history stack would become `[/, /login, /private]` and the user would be able to navigate back through the login page and to the home page + - Instead, when an action redirects, we'll use a PUSH and in this case the history stack would become `[/, /login, /private]` and the user would be able to navigate back through the login page and to the home page Note: User's can still be explicit here and use `` and the router will respect the value passed to `replace`. @@ -150,7 +150,7 @@ Note: User's can still be explicit here and use `` prop -In Remix, the `` component made an assumption that we would always restore scroll position based on `location.key`. If the key was the same as a prior location we knew the scroll position for, then we knew you had been there before and we should restore. This works great for back/forward navigations but it's a bit overly restrictive. You cannot choose to restore scroll based on anything other than `key`. +In Remix, the `` component made an assumption that we would always restore scroll position based on `location.key`. If the key was the same as a prior location we knew the scroll position for, then we knew you had been there before and we should restore. This works great for back/forward navigations, but it's a bit overly restrictive. You cannot choose to restore scroll based on anything other than `key`. -Twitter has a great implementation of this as you click around in their left nav bar - your tweet feed is always at the same place when you click back to it - even though it's a _new_ location in the history stack. This is because they're restoring by pathname here instead of `location.key`. Or maybe you want to maintain scroll position for all routes under a given pathname and you thus want to use a portion of the pathname as the scroll restoration key. +Twitter has a great implementation of this as you click around in their left nav bar — your tweet feed is always at the same place when you click back to it — even though it's a _new_ location in the history stack. This is because they're restoring by pathname here instead of `location.key`. Or maybe you want to maintain scroll position for all routes under a given pathname, and you thus want to use a portion of the pathname as the scroll restoration key. In React Router we now accept an optional `` prop where you provide a function that returns the key to use for scroll restoration: @@ -189,15 +189,15 @@ We're ok here since the new prop is optional and defaults to using `location.key ### `` prop -In addition to `` handling "restoring" scroll position on previously visited routes. It also handles "resetting" scroll position back to the top on _new_ routes. This is not always desirable if you're clicking around inside a tabbed view or something, so we've introduced a new `` prop that lets you disable the scroll reset behavior _for a given navigation_. Note that this "resetting" logic happens if and only if we cannot restore scroll to a previously known location for that scroll restoration key. +In addition to `` handling "restoring" scroll position on previously visited routes. It also handles "resetting" scroll position back to the top on _new_ routes. This is not always desirable if you're clicking around inside a tabbed view or something, so we've introduced a new `` prop that lets you disable the scroll reset behavior _for a given navigation_. Note that this "resetting" logic happens if and only if we cannot restore the scroll to a previously known location for that scroll restoration key. ### `useRevalidator()` hook -This has been a long time coming - see https://github.com/remix-run/remix/discussions/1996 🙂 +This has been a long time coming — see https://github.com/remix-run/remix/discussions/1996 🙂 ### No distinction between Error and Catch boundaries -The differentiation between error and catch proved to be a bit vague over time and a source of confusion for developers. We chose to go with just a single `errorElement` in the router for simplicity. If you throw anything, it ends up in the error boundary (available via `useRouteError`) and propagates accordingly. With this approach we leave the control in the developers hands and it's easy to maintain a similar split if desired: +The differentiation between error and catch proved to be a bit vague over time and a source of confusion for developers. We chose to go with just a single `errorElement` in the router for simplicity. If you throw anything, it ends up in the error boundary (available via `useRouteError`) and propagates accordingly. With this approach we leave the control in the developer's hands, and it's easy to maintain a similar split if desired: ```tsx function NewErrorBoundary() { @@ -232,17 +232,17 @@ We'll need to re-expose the `request.signal` as a standalone `signal` in Remix ### React-Router API surface -Initially, we chose to align closely with the existing `react-router` APIs and introduced a `` component (and it's memory/hash siblings) that would internally read the routes and create a router singleton upon first render. But as time went on we noticed some rough non-obvious foot guns with this approach, so we changed it up in [#9227][remove-singleton-pr]. Here's a few of the headaches it was causing: +Initially, we chose to align closely with the existing `react-router` APIs and introduced a `` component (and it's memory/hash siblings) that would internally read the routes and create a router singleton upon first render. But as time went on we noticed some rough non-obvious foot guns with this approach, so we changed it up in [#9227][remove-singleton-pr]. Here are a few of the headaches it was causing: - Unit tests were a pain because you need to find a way to reset the singleton in-between tests - We used a `_resetModuleScope` method for our tests - ...but this wasn't't exposed to users who may want to do their own tests around our router -- The JSX children `` objects caused non-intuitive behavior based on idiomatic react expectations +- The JSX children `` objects caused non-intuitive behavior based on idiomatic React expectations - Conditional runtime ``'s wouldn't get picked up - Adding new ``'s during local dev wouldn't get picked up during HMR - Using external state in your elements doesn't work as one might expect (see [#9225][singleton-state-issue]) -Instead, we lifted the singleton out into user-land, so that they create the router singleton and manage it outside the react tree - which is what react 18 is encouraging with `useSyncExternalStore` anyways! This also means that since users create the router - there's no longer any difference in the rendering aspect for memory/browser/hash routers (which only impacts router/history creation) - so we got rid of those and trimmed to a simple `RouterProvider`: +Instead, we lifted the singleton out into user-land, so that they create the router singleton and manage it outside the React tree — which is what react 18 is encouraging with `useSyncExternalStore` anyway! This also means that since users create the router - there's no longer any difference in the rendering aspect for memory/browser/hash routers (which only impacts router/history creation) — so we got rid of those and trimmed to a simple `RouterProvider`: ```tsx // Before diff --git a/decisions/0006-linear-workflow.md b/decisions/0006-linear-workflow.md index 6f47d1d6e9e..cb7360dff33 100644 --- a/decisions/0006-linear-workflow.md +++ b/decisions/0006-linear-workflow.md @@ -6,18 +6,18 @@ Status: accepted ## Context -The Remix development team uses Linear internally to manage and prioritize ongoing development of Remix and React-Router. In order to keep this process flowing smoothly, we document here the set of established workflows within Linear, so developers have a documented process to refer to if questions should arise. +The Remix development team uses Linear internally to manage and prioritize ongoing development of Remix and React-Router. To keep this process flowing smoothly, we document here the set of established workflows within Linear, so developers have a documented process to refer to if questions should arise. ## Linear Terms This document will use many of the following Linear terms: -- **Issue** - A ticket/task in the Linear system that describes a unit of work to be completed -- **Cycle** - A group of issues aimed to be completed within a given 1-week period (similar to a "sprint" in other Agile workflows) -- **Project** - A group of issues that comprise a larger scope of work (for example, a new feature) +- **Issue** — A ticket/task in the Linear system that describes a unit of work to be completed +- **Cycle** — A group of issues aimed to be completed within a given 1-week period (similar to a "sprint" in other Agile workflows) +- **Project** — A group of issues that comprise a larger scope of work (for example, a new feature) - Projects will almost always span multiple Cycles. -- **Status** - The state of a given Issue (Todo, In Progress, Done, etc.) -- **Assignee** - The person the Issue is currently assigned to - this indicates who is expected to take the next action to move the Issue forward +- **Status** — The state of a given Issue (Todo, In Progress, Done, etc.) +- **Assignee** — The person the Issue is currently assigned to — this indicates who is expected to take the next action to move the Issue forward - **Label** - Linear Issues can be assigned one or more Labels for filtering/searching ## Decision @@ -26,14 +26,14 @@ This document will use many of the following Linear terms: The Linear workflow is governed by the **Status** field, which has the following options: -- **Triage** - Issue is not yet reviewed and we have no idea if we'll actually do this work -- **Backlog** - Issue has been accepted as something we would like to do, but is not currently planned +- **Triage** — Issue is not yet reviewed, and we have no idea if we'll actually do this work +- **Backlog** — Issue has been accepted as something we would like to do, but is not currently planned - **Todo** - Issue has been planned for a given Cycle of work -- **In Progress** - Issue is actively being worked on in the active Cycle -- **In Review** - Issue is completed and in a PR receiving feedback -- **Needs Feedback** - Developer is blocked and needs feedback from someone else before they can be unblocked -- **Done** - Issue has been completed and merged (note this does not mean released) -- **Canceled** - Issue is not going to be done +- **In Progress** — Issue is actively being worked on in the active Cycle +- **In Review** — Issue is completed and in a PR receiving feedback +- **Needs Feedback** — Developer is blocked and needs feedback from someone else before they can be unblocked +- **Done** — Issue has been completed and merged (note this does not mean released) +- **Canceled** — Issue is not going to be done ### General Workflow @@ -57,30 +57,30 @@ graph TD In more detail, the following is the _standard_ flow of a Linear Issue: -1. When an idea or bug fix is identified through a Github Discussion/Issue/PR, we will create a Linear Issue in the **Triage** Status to track the task - 1. This should contain an appropriate title and a link to the original idea in github -2. The issue will be reviewed by the team (with final approval on larger changes and new APIs coming from Michael/Ryan) +1. When an idea or bug fix is identified through a GitHub Discussion/Issue/PR, we will create a Linear Issue in the **Triage** Status to track the task + 1. This should contain an appropriate title and a link to the original idea in GitHub +2. The team will review the issue (with final approval on larger changes and new APIs coming from Michael/Ryan) 1. If we want to move forward with the Issue, it gets moved to the **Backlog** Status - 2. If we do not want to move forward with the Issue, it get moved to the **Canceled** Status, with appropriate reasoning provided in the Linear Issue and the original Github source. -3. Periodically, Issues in the **Backlog** Status will be planned into a given cycle, and moved to the **Todo** Status + 2. If we do not want to move forward with the Issue, it gets moved to the **Canceled** Status, with appropriate reasoning provided in the Linear Issue and the original GitHub source. +3. Periodically, Issues in the **Backlog** Status will be planned into a given cycle and moved to the **Todo** Status 4. During the active Cycle, a developer will pick up a ticket to start work and will move it into the **In Progress** Status 5. Once the work is completed and a PR is opened, the developer will move the Issue to the **In Review** Status and assign it to the person who should perform the review 6. If feedback is needed, comments can be left on the PR and the ticket can switch between assignees while remaining in the **In Review** status 1. Only for larger-scale changes should an Issue be moved back to **In Progress** 7. Once merged, the Issue can be moved to the **Done** Status -As always, this is not an absolute workflow and there will be exceptions. Here's a few examples: +As always, this is not an absolute workflow, and there will be exceptions. Here are a few examples: -- If an issue opened on Github is a small bugfix, it can bypass the **Triage** status and go straight to the **Backlog** status. Or potentially even right into **Todo** if a developer has capacity to pick up the bug in the active (or upcoming) Cycle. +- If an issue opened on GitHub is a small bugfix, it can bypass the **Triage** status and go straight to the **Backlog** status. Or potentially even right into **Todo** if a developer has capacity to pick up the bug in the active (or upcoming) Cycle. - If a developer is blocked on moving forward with an Issue, it can be moved to the **Needs Feedback** Status and assigned to the person who can unblock the Issue - Sometimes an issue will be abandoned well after it was accepted and moved into the **Backlog** Status, and in these cases an Issue can always be assigned to **Canceled** ### Ongoing Processes -- Any idea/bug coming in via Github or Discord (or elsewhere) can be put into an Issue in the **Triage** status for review +- Any idea/bug coming in via GitHub or Discord (or elsewhere) can be put into an Issue in the **Triage** status for review - Michael and Ryan should review the **Triage** Issues on a regular (weekly?) basis and move tickets to **Backlog** or **Canceled** - If tickets are accepted, they should be given appropriate Labels and/or Projects - - If tickets are rejected, we should provide the rationale in the Linear ticket and on the original source (i.e., github) + - If tickets are rejected, we should provide the rationale in the Linear ticket and on the original source (i.e., GitHub) - Michael/Ryan (and team?) will decide what to work on in a given Cycle and move tickets from **Backlog** to **Todo** and assign a proper Cycle - All team members should review any tickets assigned to them on a regular (daily?) basis and provide reviews/feedback as needed to keep tickets moving along - All team members should work from their **Todo** queue during active Cycles diff --git a/decisions/0007-remix-on-react-router-6-4-0.md b/decisions/0007-remix-on-react-router-6-4-0.md index 7d925c4299d..48596d320d7 100644 --- a/decisions/0007-remix-on-react-router-6-4-0.md +++ b/decisions/0007-remix-on-react-router-6-4-0.md @@ -8,7 +8,7 @@ Status: accepted Now that we're almost done [Remixing React Router][remixing-react-router] and will be shipping `react-router@6.4.0` shortly, it's time for us to start thinking about how we can layer Remix on top of the latest React Router. This will allow us to delete a _bunch_ of code from Remix for handling the Data APIs. This document aims to discuss the changes we foresee making and some potential iterative implementation approaches to avoid a big-bang merge. -From an iterative-release viewpoint, there's 4 separate "functional" aspects to consider here: +From an iterative-release viewpoint, there are four separate "functional" aspects to consider here: 1. Server data loading 2. Server react component rendering @@ -19,7 +19,7 @@ From an iterative-release viewpoint, there's 4 separate "functional" aspects to ## Decision -The high level approach is as follows +The high-level approach is as follows 1. SSR data loading 1. Update `handleResourceRequest` to use `createStaticHandler` behind a flag @@ -37,23 +37,23 @@ The high level approach is as follows ## Details -There are 2 main areas where we have to make changes: +There are two main areas where we have to make changes: 1. Handling server-side requests in `@remix-run/server-runtime` (mainly in the `server.ts` file) -2. Handling client-side hydration + routing in `@remix-run/react` (mainly in the `components.ts`, `server.ts` and `browser.ts` files) +2. Handling client-side hydration and routing in `@remix-run/react` (mainly in the `components.ts`, `server.ts` and `browser.ts` files) Since these are separated by the network chasm, we can actually implement these independent of one another for smaller merges, iterative development, and easier rollbacks should something go wrong. ### Do the server data-fetching migration first -There's two primary reasons it makes sense to handle the server-side data-fetching logic first: +There are two primary reasons it makes sense to handle the server-side data-fetching logic first: -1. It's a smaller surface area change since there's effectively only 1 new API to work with in `createStaticHandler` +1. It's a smaller surface area change since there's effectively only one new API to work with in `createStaticHandler` 2. It's easier to implement in a feature-flagged manner since we're on the server and bundle size is not a concern We can do this on the server using the [strangler pattern][strangler-pattern] so that we can confirm the new approach is functionally equivalent to the old approach. Depending on how far we take it, we can assert this through unit tests, integration tests, as well as run-time feature flags if desired. -For example, pseudo code for this might look like the following, where we enable via a flag during local development and potentially unit/integration tests. We can throw exceptions anytime the new static handler results in different SSR data. Once we're confident, we delete the current code and remove the flag conditional. +For example, pseudocode for this might look like the following, where we enable via a flag during local development and potentially unit/integration tests. We can throw exceptions anytime the new static handler results in different SSR data. Once we're confident, we delete the current code and remove the flag conditional. ```tsx // Runtime-agnostic flag to enable behavior, will always be committed as @@ -114,7 +114,7 @@ We can also split this into iterative approaches on the server too, and do `hand #### Notes - This can't use `process.env` since the code we're changing is runtime agnostic. We'll go with a local hardcoded variable in `server.ts` for now to avoid runtime-specific ENV variable concerns. - - Unit and integration tests may need to have their own copies of this variable as well to remain passing. For example, we have unit tests that assert that a loader is called once for a given route - but when this flag is enabled, that loader will be called twice so we can set up a conditional assertion based on the flag. + - Unit and integration tests may need to have their own copies of this variable as well to remain passing. For example, we have unit tests that assert that a `loader` is called once for a given route — but when this flag is enabled, that `loader` will be called twice, so we can set up a conditional assertion based on the flag. - The `remixContext` sent through `entry.server.ts` will be altered in shape. We consider this an opaque API so not a breaking change. #### Implementation approach @@ -139,7 +139,7 @@ We can also split this into iterative approaches on the server too, and do `hand ### Do the UI rendering layer second -The rendering layer in `@remix-run/react` is a bit more of a whole-sale replacement and comes with backwards-compatibility concerns, so it makes sense to do second. However, we can still do this iteratively, we just can't deploy iteratively since the SSR and client HTML need to stay synced (and associated hooks need to read from the same contexts). First, we can focus on getting the SSR document rendered properly without ``. Then second we'll add in client-side hydration. +The rendering layer in `@remix-run/react` is a bit more of a wholesale replacement and comes with backwards-compatibility concerns, so it makes sense to do second. However, we can still do this iteratively, we just can't deploy iteratively since the SSR and client HTML need to stay synced (and associated hooks need to read from the same contexts). First, we can focus on getting the SSR document rendered properly without ``. Then second, we'll add in client-side hydration. The main changes here include: diff --git a/decisions/0008-only-support-js-conversion-for-app-code.md b/decisions/0008-only-support-js-conversion-for-app-code.md index 677fce3755f..6f45bfa81db 100644 --- a/decisions/0008-only-support-js-conversion-for-app-code.md +++ b/decisions/0008-only-support-js-conversion-for-app-code.md @@ -6,9 +6,9 @@ Status: accepted ## Context -Remix defaults to Typescript, but some users prefer to use Javascript. +Remix defaults to TypeScript, but some users prefer to use JavaScript. When creating a new Remix project via `npx create-remix` today, the CLI asks the user if they'd -prefer to use Typescript or Javascript. +prefer to use TypeScript or JavaScript. ```sh ❯ npx create-remix@latest @@ -21,15 +21,15 @@ prefer to use Typescript or Javascript. ``` Originally, this was implemented by having separate variants of a template for TS and JS. -This worked, but duplicated huge portions of templates making those variants hard to maintain. +This worked but duplicated huge portions of templates, making those variants hard to maintain. To fix this, the decision was made to maintain _only_ the TS variant of each template. To provide JS-only Remix projects, the Remix CLI would copy the TS template and then dynamically -convert all Typescript-related code to their Javascript equivalents. +convert all Typescript-related code to their JavaScript equivalents. While converting the code in the Remix app directory (e.g. `app/`) is reliable, conversion of -TS-related code outside of the `app/` directory are tricky and error-prone. +TS-related code outside the `app/` directory are tricky and error-prone. -### 1 Stale references to `.ts` files +### 1. Stale references to `.ts` files For example, both the [Indie][indie-stack] and [Blues][blues-stack] stacks have broken scripts when selecting `Javascript`, because they still reference `server.ts` and `seed.ts`. @@ -37,7 +37,7 @@ They also still reference TS-only tools like `ts-node` in their scripts. ### 2.a MJS -When transpiling code outside of the `app/` directory from TS to JS, the easiest thing to do is +When transpiling code outside the `app/` directory from TS to JS, the easiest thing to do is to convert to ESM-style `.mjs` since Remix app code already uses ESM. ESM-style JS requires one of the following: @@ -46,7 +46,7 @@ ESM-style JS requires one of the following: - b) Use `.mjs` extension (a) immediately breaks Remix's builds since the settings in `package.json` apply to app code, not just code and -scripts outside of the app directory. +scripts outside the app directory. (b) seems more promising, but `.mjs` files require file extensions for import specifiers. Reliably changing relative imports without extensions to have the proper extensions is @@ -66,32 +66,32 @@ Maybe there's a way to do this, but the complexity cost seems high. ### 2.b CJS -If we don't use the `.mjs` extension, Node will default to treating scripts and code outside of the app directory +If we don't use the `.mjs` extension, Node will default to treating scripts and code outside the app directory as CJS. Since CJS doesn't support ESM-style `import`/`export`, we'd then need to convert all `import`s and `export`s to their equivalent `require`/`module.exports`. -Important to remember that the converted code is meant to be read and edited by other developers, so its not acceptable +Important to remember that the converted code is meant to be read and edited by other developers, so it's not acceptable to produce a bunch of boilerplate adapters for the imports and exports as would be typical in build output. -Converting the imports and exports may be doable, but again, carries high complexity cost. +Converting the imports and exports may be doable, but again, it carries high complexity cost. ## Decision -Only support JS conversion for app code, not for scripts or code outside of the Remix app directory. +Only support JS conversion for app code, not for scripts or code outside the Remix app directory. ## Consequences Users will have three options: -1. Use a Typescript template -2. Use a Typescript template with app directory converted to JS +1. Use a TypeScript template +2. Use a TypeScript template with the app directory converted to JS 3. Use a dedicated Javascript template ### Converting remaining TS to JS If you don't like the remaining TS from option (2) and cannot find a suitable template for option (3), -you can still remove any remaining Typescript code manually: +you can still remove any remaining TypeScript code manually: - Remove `tsconfig.json` or replace it with the equivalent `jsconfig.json` - Replace TS-only tools with their JS counterparts (e.g. `ts-node` -> `node`) diff --git a/decisions/0009-do-not-rely-on-treeshaking-for-correctness.md b/decisions/0009-do-not-rely-on-treeshaking-for-correctness.md index f007f258daa..dcdb2f7fc46 100644 --- a/decisions/0009-do-not-rely-on-treeshaking-for-correctness.md +++ b/decisions/0009-do-not-rely-on-treeshaking-for-correctness.md @@ -1,4 +1,4 @@ -# Do not rely on treeshaking for correctness +# Do not rely on tree shaking for correctness Date: 2024-01-02 @@ -9,7 +9,7 @@ Status: accepted Remix lets you write code that runs on both the server and the client. For example, it's common to have server and client code in the same route file. While blending server and client code is convenient, Remix needs to ensure that server-only code is never shipped to the client. -This prevents secrets available to the server from leaking into client and also prevents the client from crashing due to code that expects a server environment. +This prevents secrets available to the server from leaking into a client and also prevents the client from crashing due to code that expects a server environment. Server-only code comes in three forms: @@ -17,20 +17,20 @@ Server-only code comes in three forms: 2. Imports from `.server` directories or `.server` files. 3. Imports from server-only packages like `node:fs` -Remix previously relied on treeshaking to exclude server-only code. +Remix previously relied on tree shaking to exclude server-only code. Specifically, Remix used [virtual modules for each route][virtual-modules] that re-exported client-safe exports from the route. For a brief stint, Remix instead used [AST transforms][ast-transforms] to remove server-only exports. In either case, Remix would remove server exports from routes and then let the bundler treeshake any unused code and dependencies from the client bundle. -The main benefit is that server and client code can co-exist in the same module graph and even the same file -and the treeshaking saves you from the tedium of explicitly marking or separating server-only code yourself. +The main benefit is that server and client code can co-exist in the same module graph and even the same file, +and the tree shaking saves you from the tedium of explicitly marking or separating server-only code yourself. ### Human error -However, this is also the main drawback; "server-only" is implicit and must be inferred by thorough treeshaking. -Even if treeshaking were perfect, this approach still leaves the door open to human error. +However, this is also the main drawback; "server-only" is implicit and must be inferred by thorough tree shaking. +Even if tree shaking were perfect, this approach still leaves the door open to human error. If you or anyone on your team accidentally references server-only code from client code, the bundler will happily include that code in the client. -You won't get any indication of this at build time, but only at runtime. +You won't get any sign of this at build time but only at runtime. Your app could crash when trying to execute code meant for the server, or worse, you could accidentally ship secrets to the client. ### `.server` modules @@ -40,44 +40,44 @@ Any modules with a directory named `.server` are never allowed into the module g Similarly, files with a `.server` extension are also excluded from the client module graph. Theoretically, `.server` modules are a redundancy. -A perfect module graph with perfect treeshaking shouldn't _need_ `.server` modules. +A perfect module graph with perfect tree shaking shouldn't _need_ `.server` modules. But in practice, `.server` modules are indispensable. They are the only guaranteed way to exclude code from the client. ### An imperfect optimization -As we already discussed, even if treeshaking were perfect, it would still be a bad idea to rely on it to exclude server-only code. -But treeshaking is a hard problem, especially in a language as dynamic as JavaScript. -In the real world, treeshaking is not perfect. +As we already discussed, even if tree shaking were perfect, it would still be a bad idea to rely on it to exclude server-only code. +But tree shaking is a hard problem, especially in a language as dynamic as JavaScript. +In the real world, tree shaking is not perfect. -That's why treeshaking is designed to be an _optimization_ that slims down your bundle. -Your code should already be correct before treeshaking is applied. -Bundlers are allowed to make [their own tradeoffs about how much treeshaking][esbuild-minify-considerations] they want to do. +That's why tree shaking is designed to be an _optimization_ that slims down your bundle. +Your code should already be correct before tree shaking is applied. +Bundlers are allowed to make [their own tradeoffs about how much tree shaking][esbuild-minify-considerations] they want to do. And that shouldn't [affect Remix's implementation][remix-minify-syntax]. -They are even allowed to do _less_ treeshaking without needing a major version bump. +They are even allowed to do _less_ tree shaking without needing a major version bump. -Additionally, code can only be treeshaken if it is known to be side-effect free. -Unfortunately, even fully side-effect free packages often omit `sideEffects: false` from their `package.json`. -And sometimes side-effects are desired! -What if there's a server-side package with side-effects that we want to include in our server bundle? +Additionally, code can only be tree shaken if it is known to be side-effect-free. +Unfortunately, even fully side effect free packages often omit `sideEffects: false` from their `package.json`. +And sometimes side effects are desired! +What if there's a server-side package with side effects that we want to include in our server bundle? How could we exclude that from the client bundle? There are ways, but they're all hacky and brittle. ### Vite -Remix is becoming a Vite plugin, but Vite's on-demand compilation in dev is incompatible with treeshaking. +Remix is becoming a Vite plugin, but Vite's on-demand compilation in dev is incompatible with tree shaking. Since the compilation is on-demand, Vite only knows the _current_ importer for the module, not all possible importers. ### Summary -- Even if treeshaking were perfect, it leaves the door open for human error +- Even if tree shaking were perfect, it leaves the door open for human error - `.server` modules guarantee that server-only code is excluded from the client -- Treeshaking is an imperfect _optimization_, so a Remix app should work correctly and exclude server-only code even without treeshaking -- Vite's architecture makes treeshaking in dev untenable +- Tree shaking is an imperfect _optimization_, so a Remix app should work correctly and exclude server-only code even without tree shaking +- Vite's architecture makes tree shaking in dev untenable ## Decision -Do not rely on implicit, cross-module treeshaking for correctness. +Do not rely on implicit, cross-module tree shaking for correctness. Instead: - Forcibly remove server-only route exports and then explicitly run a dead-code elimination pass diff --git a/decisions/0010-splitting-up-client-and-server-code-in-vite.md b/decisions/0010-splitting-up-client-and-server-code-in-vite.md index 70755110d71..4c9fc8bcfb1 100644 --- a/decisions/0010-splitting-up-client-and-server-code-in-vite.md +++ b/decisions/0010-splitting-up-client-and-server-code-in-vite.md @@ -6,9 +6,9 @@ Status: accepted ## Context -Before adopting Vite, Remix used to rely on ESbuild's treeshaking to implicitly separate client and server code. -Even though Vite provides equivalent treeshaking (via Rollup) for builds, it does not perform cross-module treeshaking when running the dev server. -In any case, we think its a [bad idea to rely on treeshaking for correctness][decision-0009]. +Before adopting Vite, Remix used to rely on ESbuild's tree shaking to implicitly separate client and server code. +Even though Vite provides equivalent tree shaking (via Rollup) for builds, it does not perform cross-module tree shaking when running the dev server. +In any case, we think it's a [bad idea to rely on tree shaking for correctness][decision-0009]. Goals: @@ -61,7 +61,7 @@ The Plugin API makes this apparent:[^1] - `resolveId` only provides the current `importer` - `load` and `transform` do not receive any information about the module graph -This approach lets Vite load and transform each module _once_ and cache the result[^2] which is a keystone for its speed. +This approach lets Vite load and transform each module _once_ and cache the result[^2], which is a keystone for its speed. #### Handling mixed modules @@ -69,10 +69,10 @@ While `.server` modules are a great way to separate client and server code in mo there will always be a need to stitch together modules that mix client and server code. For example, you may want to migrate from the previous compiler to Vite without needing to manually split up mixed modules. -But supporting mixed modules directly in Remix would require compile-time magic which would add substantial complexity. +But supporting mixed modules directly in Remix would require compile-time magic, which would add substantial complexity. Not only would it degrade performance for all users (Goal 4 ❌), but writing compile-time transforms that manipulate the AST is much more error-prone than throwing a compile-time error when `.server` modules are imported by client code. -Depending on how its implemented, bugs in that compile-time magic could open the door to leaking server code into the client (Goal 1 ❌). +Depending on how it's implemented, bugs in that compile-time magic could open the door to leaking server code into the client (Goal 1 ❌). ## Decision