Skip to content

Commit

Permalink
Merge branch 'dev' into feature/configure-timeout-for-livereload
Browse files Browse the repository at this point in the history
  • Loading branch information
joelazar authored Oct 17, 2022
2 parents 74fc111 + a0823ed commit a82cb1c
Show file tree
Hide file tree
Showing 20 changed files with 883 additions and 43 deletions.
8 changes: 8 additions & 0 deletions .changeset/large-colts-drop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"remix": patch
"@remix-run/dev": patch
---

add CSS plugin to `esbuild` so that any assets in css files are also copied (and hashed) to the `assetsBuildDirectory`

currently if you import a css file that has `background: url('./relative.png');` the `relative.png` file is not copied to the build directory, which is a problem when dealing with npm packages that have css files with font files in them like fontsource
6 changes: 6 additions & 0 deletions .changeset/lovely-cats-jump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"remix": patch
"@remix-run/react": patch
---

make `<Form />` respect the formMethod attribute set on the submitter (the button submitting the form).
6 changes: 6 additions & 0 deletions .changeset/many-gorillas-mix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"remix": patch
"@remix-run/dev": patch
---

Add proxy support to `create-remix` cli
6 changes: 6 additions & 0 deletions .changeset/tame-hotels-attack.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"remix": patch
"@remix-run/dev": patch
---

Allow private GitHub repos to use the latest release url for tarballs when using `create-remix`
5 changes: 5 additions & 0 deletions contributors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
- fgiuliani
- fishel-feng
- francisudeji
- freeman
- frontsideair
- fx109138
- gabimor
Expand Down Expand Up @@ -195,6 +196,7 @@
- jodygeraldo
- joelazar
- johannesbraeunig
- johnmberger
- johnpolacek
- johnson444
- joms
Expand Down Expand Up @@ -222,11 +224,13 @@
- KenanYusuf
- kentcdodds
- kevinrambaud
- kevlened
- kgregory
- kiancross
- kilian
- kiliman
- kimdontdoit
- KingSora
- klauspaiva
- knowler
- konradkalemba
Expand Down Expand Up @@ -281,6 +285,7 @@
- maxprilutskiy
- mbarto
- mcansh
- mdoury
- medayz
- meetbryce
- mehulmpt
Expand Down
166 changes: 166 additions & 0 deletions decisions/0007-remix-on-react-router-6-4-0.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
# Layering Remix on top of React Router 6.4

Date: 2022-08-16

Status: accepted

## Context

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:

1. Server data loading
2. Server react component rendering
3. Client hydration
4. Client data loading

(1) can be implemented and deployed in isolation. (2) and (3) need to happen together since the contexts/components need to match. And (4) comes for free since the loaders/actions will be included on the routes we create in (3).

## Decision

The high level approach is as follows

1. SSR data loading
1. Update `handleResourceRequest` to use `createStaticHandler` behind a flag
1. Aim to get unit and integration tests asserting both flows if possible
2. Update `handleDataRequest` in the same manner
3. Update `handleDocumentRequest` in the same manner
1. Confirm unit and integration tests are all passing
4. Write new `RemixContext` data into `EntryContext` and remove old flow
2. Deploy `@remix-run/server-runtime` changes once comfortable
3. Handle `@remix-run/react` in a short-lived feature branch
1. server render without hydration (replace `EntryContext` with `RemixContext`)
2. client-side hydration
3. add backwards compatibility changes
4. Deploy `@remix-run/react` changes once comfortable

## Details

There are 2 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)

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:

1. It's a smaller surface area change since there's effectively only 1 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.

```js
// Runtime-agnostic flag to enable behavior, will always be committed as
// `false` initially, and toggled to true during local dev
const ENABLE_REMIX_ROUTER = false;

async function handleDocumentRequest({ request }) {
let appState = {
trackBoundaries: true,
trackCatchBoundaries: true,
catchBoundaryRouteId: null,
renderBoundaryRouteId: null,
loaderBoundaryRouteId: null,
error: undefined,
catch: undefined,
};

// ... do all the current stuff

let serverHandoff = {
actionData,
appState: appState,
matches: entryMatches,
routeData,
};

let entryContext = {
...serverHandoff,
manifest: build.assets,
routeModules,
serverHandoffString: createServerHandoffString(serverHandoff),
};

// If the flag is enabled, process the request again with the new static
// handler and confirm we get the same data on the other side
if (ENABLE_REMIX_ROUTER) {
let staticHandler = unstable_createStaticHandler(routes);
let context = await staticHandler.query(request);

// Note: == only used for brevity ;)
assert(entryContext.matches === context.matches);
assert(entryContext.routeData === context.loaderData);
assert(entryContext.actionData === context.actionData);

if (catchBoundaryRouteId) {
assert(appState.catch === context.errors[catchBoundaryRouteId]);
}

if (loaderBoundaryRouteId) {
assert(appState.error === context.errors[loaderBoundaryRouteId]);
}
}
}
```

We can also split this into iterative approaches on the server too, and do `handleResourceRequest`, `handleDataRequest`, and `handleDocumentRequest` independently (either just implementation or implementation + release). Doing them in that order would also likely go from least to most complex.

#### 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.
- 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

1. Use `createHierarchicalRoutes` to build RR `DataRouteObject` instances
1. See `createStaticHandlerDataRoutes` in the `brophdawg11/rrr` branch
2. Create a static handler per-request using `unstable_createStaticHandler`
3. `handleResourceRequest`
1. This one should be _really_ simple since it should just send back the raw `Response` from `queryRoute`
4. `handleDataRequest`
1. This is only slightly more complicated than resource routes, as it needs to handle serializing errors and processing redirects into 204 Responses for the client
5. `handleDocumentRequest`
1. This is the big one. It simplifies down pretty far, but has the biggest surface area where some things don't quite match up
2. We need to map query "errors" to Remix's definition of error/catch and bubble them upwards accordingly.
1. For example, in a URL like `/a/b/c`, if C exports a a `CatchBoundary` but not an `ErrorBoundary`, then it'll be represented in the `DataRouteObject` with `hasErrorBoundary=true` since the `@remix-run/router` doesn't distinguish
2. If C's loader throws an error, the router will "catch" that at C's `errorElement`, but we then need to re-bubble that upwards to the nearest `ErrorBoundary`
3. See `differentiateCatchVersusErrorBoundaries` in the `brophdawg11/rrr` branch
3. New `RemixContext`
1. `manifest`, `routeModules`, `staticHandlerContext`, `serverHandoffString`
2. Create this alongside `EntryContext` assert the values match
4. If we catch an error during render, we'll have tracked the boundaries on `staticHandlerContext` and can use `getStaticContextFromError` to get a new context for the second pass (note the need to re-call `differentiateCatchVersusErrorBoundaries`)

### 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 `<Scripts/>`. Then second we'll add in client-side hydration.

The main changes here include:

- Removal of `RemixEntry` and it's context in favor of a new `RemixContext.Provider` wrapping `DataStaticRouter`/`DataBrowserRouter`
- All this context needs is the remix-specific aspects (`manifest`, `routeModules`)
- Everything else from the old RemixEntryContext is now in the router contexts (and `staticHandlerContext` during SSR)
- Some aspects of `@remix-run/react`'s `components.tsx` file are now fully redundant and can be removed completely in favor of re-exporting from `react-router-dom`:
- `Form`, `useFormAction`, `useSubmit`, `useMatches`, `useFetchers`
- Other aspects are largely redundant but need some Remix-specific things, so these will require some adjustments:
- `Link`, `useLoaderData`, `useActionData`, `useTransition`, `useFetcher`

#### Backwards Compatibility Notes

- `useLoaderData`/`useActionData` need to retain their generics, and are not currently generic in `react-router`
- `useTransition` needs `submission` and `type` added
- `<Form method="get">` no longer goes into a "submitting" state in `react-router-dom`
- `useFetcher` needs `type` added
- `unstable_shouldReload` replaced by `shouldRevalidate`
- Can we use it if it's there but prefer `shouldRevalidate`?
- Distinction between error and catch boundaries
- `Request.signal` - continue to send separate `signal` param

[remixing-react-router]: https://remix.run/blog/remixing-react-router
[strangler-pattern]: https://martinfowler.com/bliki/StranglerFigApplication.html
1 change: 1 addition & 0 deletions docs/other-api/dev.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ remix create ./my-app --template :username/:repo
remix create ./my-app --template https://github.com/:username/:repo
remix create ./my-app --template https://github.com/:username/:repo/tree/:branch
remix create ./my-app --template https://github.com/:username/:repo/archive/refs/tags/:tag.tar.gz
remix create ./my-app --template https://github.com/:username/:repo/releases/latest/download/:tag.tar.gz
remix create ./my-app --template https://example.com/remix-template.tar.gz
```

Expand Down
44 changes: 42 additions & 2 deletions integration/compiler-test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import path from "path";
import fs from "fs/promises";
import fse from "fs-extra";
import { test, expect } from "@playwright/test";
import { PassThrough } from "stream";

Expand All @@ -9,6 +9,7 @@ import {
js,
json,
createFixtureProject,
css,
} from "./helpers/create-fixture";
import type { Fixture, AppFixture } from "./helpers/create-fixture";
import { PlaywrightFixture } from "./helpers/playwright-fixture";
Expand Down Expand Up @@ -89,6 +90,18 @@ test.describe("compiler", () => {
return <div id="package-with-submodule">{submodule()}</div>;
}
`,
"app/routes/css.jsx": js`
import stylesUrl from "@org/css/index.css";
export function links() {
return [{ rel: "stylesheet", href: stylesUrl }]
}
export default function PackageWithSubModule() {
return <div id="package-with-submodule">{submodule()}</div>;
}
`,

"remix.config.js": js`
let { getDependenciesToBundle } = require("@remix-run/dev");
module.exports = {
Expand Down Expand Up @@ -156,6 +169,22 @@ test.describe("compiler", () => {
return "package-with-submodule";
}
`,
"node_modules/@org/css/package.json": json({
name: "@org/css",
version: "1.0.0",
main: "index.css",
}),
"node_modules/@org/css/font.woff2": "font",
"node_modules/@org/css/index.css": css`
body {
background: red;
}
@font-face {
font-family: "MyFont";
src: url("./font.woff2");
}
`,
},
});

Expand Down Expand Up @@ -266,6 +295,17 @@ test.describe("compiler", () => {
);
});

test("copies imports in css files to assetsBuildDirectory", async () => {
let buildDir = path.join(fixture.projectDir, "public", "build", "_assets");
let files = await fse.readdir(buildDir);
expect(files).toHaveLength(2);

let cssFile = files.find((file) => file.match(/index-[a-z0-9]{8}\.css/i));
let fontFile = files.find((file) => file.match(/font-[a-z0-9]{8}\.woff2/i));
expect(cssFile).toBeTruthy();
expect(fontFile).toBeTruthy();
});

// TODO: remove this when we get rid of that feature.
test("magic imports still works", async () => {
let magicExportsForNode = [
Expand Down Expand Up @@ -314,7 +354,7 @@ test.describe("compiler", () => {
"useSubmit",
"useTransition",
];
let magicRemix = await fs.readFile(
let magicRemix = await fse.readFile(
path.resolve(fixture.projectDir, "node_modules/remix/dist/index.js"),
"utf8"
);
Expand Down
Loading

0 comments on commit a82cb1c

Please sign in to comment.