-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dev): HMR + Hot Data Revalidation #5259
Merged
Merged
Changes from all commits
Commits
Show all changes
67 commits
Select commit
Hold shift + click to select a range
df0d4a0
wip
pcattori 6a9a5ff
wip
pcattori 44c1fbb
wip
pcattori 2c2b322
IT LIVES
jacob-ebey 5b31a3e
revert port
jacob-ebey fd5bd1d
Updated to completely strip server methods from route modules instead…
jacob-ebey cbccdcf
- remove logs
jacob-ebey f3d4992
- added a few comments
jacob-ebey 29c20f3
consistent timestamps for entry and routes
pcattori 0430bec
wip
pcattori 56df876
smart hmr: live reload when server needs it, otherwise hmr
pcattori 5cb03a4
CSS HMR ✅
jacob-ebey 78ea28d
update in better location to avoid race conditions
jacob-ebey faa371c
comment things a bit
jacob-ebey 22fc2c9
more comments
jacob-ebey e01b9e5
always HMR, full reloads are no longer needed
jacob-ebey cf7ec3f
detect loader changes for router self-revalidation
pcattori 628062b
refactor: compiler returns assets manifest and metafiles for browser/…
pcattori de9e5cd
refactor: basing hmr updates off of assets manifest routes
pcattori e9d81ff
fix: update react scripts component to use `hmr.runtime`
pcattori 8ba7b32
refactor: manifest timestamp
pcattori 6d858bd
fix: prevent duplicate chunks caused by untimestamped imports
pcattori 54b26e9
feat: log hmr updates including reason and if revalidating
pcattori 6f1770b
refactor: hmr messages
pcattori 54b4476
fix: do not preload route chunk if hmr already loaded it
pcattori 1dc778a
refactor(dev): update react-router api call for hmr
pcattori 9f2cb05
refactor: manually revalidate until we have more granular apis
pcattori bf296b7
fix(dev): only do hmr for unstable_dev
pcattori 5449aa0
fix(dev): dynamic, configurable unstable_dev port
pcattori a5acb65
fix(dev): only apply hmr for browser routes in dev mode
pcattori a0a7643
wip: test
pcattori ddeeeda
fix: detect `loader` changes when using variable expressions
pcattori 5fdd600
fix: chunk hmr runtime separately
pcattori c0d7483
wip: updated typescript so that transforms work in tests
pcattori 421df1f
fix: hash hmr routes as part of assets manifest version
pcattori 5bfef5d
chore: update entry to more simply support refresh
jacob-ebey d84e68f
add abort controller for subsequent HMR updates before completion
jacob-ebey e926d17
test(dev): dynamic ports for hmr integration test
pcattori 35b4fec
fix: hmr caused subsequent navigations to fail
jacob-ebey 98f1062
remove comment
jacob-ebey 98a44c0
revert module changes in favor of updating cache early and in place
jacob-ebey 026b1c4
strip `import.meta` from cjs builds
jacob-ebey a74aee4
test(dev): `import.meta` support for jest
pcattori 764731f
fix(server-runtime): fix typechecking
pcattori 5974015
style(dev): indentation
pcattori 4b87d03
test(dev): reference local remix dev cli
pcattori d2b2d14
fix(dev): only send hmr updates for files that changed
pcattori 69bc58f
refactor(dev): list of isolated chunks for hmr
pcattori b2bcad9
wip: esbuild loaders for browser route mods
pcattori f3ede9e
refactor(dev): extract transform utils from `codemod/`
pcattori 5cc7ea1
refactor(dev): remove server-only exports via babel ast transforms
pcattori 8c057a2
chore(dev): remove unused dependency
pcattori 1ce71c2
fix(dev): transform jsx
pcattori acd3894
fix(dev): mdx browser routes
pcattori 1832413
chore: update yarn.lock
pcattori 686a6e9
chore(react): set `preventAssignment` to silence warning
pcattori 24a9587
chore: add changset for hmr
pcattori 0ec995c
update router to experimental
jacob-ebey de5f190
chore: depend on pre-releases instead of experimentals
pcattori 491726f
test(dev): increase timeouts for hmr test
pcattori 3366bc9
test(dev): increase timeout for windows ci
pcattori 939a30c
test(dev): increase timeout to 1m for windows ci
pcattori 14e8110
test(dev): increase timeout to 2m for windows ci
pcattori ac53b8b
Merge branch 'dev' into pedro/hmr-adventures
pcattori 8f60539
test(dev): fail immediately when stderr is written to
pcattori 7068fae
test(dev): `cross-env` for `NODE_ENV` on windows
pcattori 3121ea6
test(dev): revert timeout for windows ci
pcattori File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
--- | ||
"@remix-run/dev": minor | ||
"@remix-run/react": minor | ||
"@remix-run/server-runtime": minor | ||
--- | ||
|
||
Hot Module Replacement and Hot Data Revalidation | ||
|
||
- Requires `unstable_dev` future flag to be enabled | ||
- HMR provided through React Refresh | ||
|
||
Features: | ||
- HMR for component and style changes | ||
- HDR when loaders for current route change | ||
|
||
Known limitations for MVP: | ||
- Only implemented for React via React Refresh | ||
- No `import.meta.hot` API exposed yet | ||
- Revalidates _all_ loaders on route when loader changes are detected | ||
- Loader changes do not account for imported dependencies changing | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,363 @@ | ||
import { test, expect } from "@playwright/test"; | ||
import execa from "execa"; | ||
import fs from "node:fs"; | ||
import path from "node:path"; | ||
import type { Readable } from "node:stream"; | ||
import getPort, { makeRange } from "get-port"; | ||
|
||
import { createFixtureProject } from "./helpers/create-fixture"; | ||
|
||
let fixture = (options: { port: number; appServerPort: number }) => ({ | ||
future: { | ||
unstable_dev: { | ||
port: options.port, | ||
appServerPort: options.appServerPort, | ||
}, | ||
unstable_tailwind: true, | ||
}, | ||
files: { | ||
"package.json": ` | ||
{ | ||
"private": true, | ||
"sideEffects": false, | ||
"scripts": { | ||
"dev:remix": "cross-env NODE_ENV=development node ./node_modules/@remix-run/dev/dist/cli.js dev", | ||
"dev:app": "cross-env NODE_ENV=development nodemon --watch build/ ./server.js" | ||
}, | ||
"dependencies": { | ||
"@remix-run/node": "0.0.0-local-version", | ||
"@remix-run/react": "0.0.0-local-version", | ||
"cross-env": "0.0.0-local-version", | ||
"express": "0.0.0-local-version", | ||
"isbot": "0.0.0-local-version", | ||
"nodemon": "0.0.0-local-version", | ||
"react": "0.0.0-local-version", | ||
"react-dom": "0.0.0-local-version", | ||
"tailwindcss": "0.0.0-local-version" | ||
}, | ||
"devDependencies": { | ||
"@remix-run/dev": "0.0.0-local-version", | ||
"@types/react": "0.0.0-local-version", | ||
"@types/react-dom": "0.0.0-local-version", | ||
"typescript": "0.0.0-local-version" | ||
}, | ||
"engines": { | ||
"node": ">=14" | ||
} | ||
} | ||
`, | ||
"server.js": ` | ||
let path = require("path"); | ||
let express = require("express"); | ||
let { createRequestHandler } = require("@remix-run/express"); | ||
|
||
const app = express(); | ||
app.use(express.static("public", { immutable: true, maxAge: "1y" })); | ||
|
||
const MODE = process.env.NODE_ENV; | ||
const BUILD_DIR = path.join(process.cwd(), "build"); | ||
|
||
app.all( | ||
"*", | ||
createRequestHandler({ | ||
build: require(BUILD_DIR), | ||
mode: MODE, | ||
}) | ||
); | ||
|
||
let port = ${options.appServerPort}; | ||
app.listen(port, () => { | ||
require(BUILD_DIR); | ||
console.log('✅ app ready: http://localhost:' + port); | ||
}); | ||
`, | ||
"tailwind.config.js": ` | ||
/** @type {import('tailwindcss').Config} */ | ||
module.exports = { | ||
content: ["./app/**/*.{ts,tsx,jsx,js}"], | ||
theme: { | ||
extend: {}, | ||
}, | ||
plugins: [], | ||
}; | ||
`, | ||
"app/tailwind.css": ` | ||
@tailwind base; | ||
@tailwind components; | ||
@tailwind utilities; | ||
`, | ||
"app/root.tsx": ` | ||
import type { LinksFunction } from "@remix-run/node"; | ||
import { Link, Links, LiveReload, Meta, Outlet, Scripts } from "@remix-run/react"; | ||
|
||
import Counter from "./components/counter"; | ||
import styles from "./tailwind.css"; | ||
|
||
export const links: LinksFunction = () => [ | ||
{ rel: "stylesheet", href: styles }, | ||
]; | ||
|
||
export default function Root() { | ||
return ( | ||
<html lang="en" className="h-full"> | ||
<head> | ||
<Meta /> | ||
<Links /> | ||
</head> | ||
<body className="h-full"> | ||
<header> | ||
<label htmlFor="root-input">Root Input</label> | ||
<input id="root-input" /> | ||
<Counter id="root-counter" /> | ||
<nav> | ||
<ul> | ||
<li><Link to="/">Home</Link></li> | ||
<li><Link to="/about">About</Link></li> | ||
</ul> | ||
</nav> | ||
</header> | ||
<Outlet /> | ||
<Scripts /> | ||
<LiveReload /> | ||
</body> | ||
</html> | ||
); | ||
} | ||
`, | ||
"app/routes/index.tsx": ` | ||
import { useLoaderData } from "@remix-run/react"; | ||
export default function Index() { | ||
const t = useLoaderData(); | ||
return ( | ||
<main> | ||
<h1>Index Title</h1> | ||
</main> | ||
) | ||
} | ||
`, | ||
"app/routes/about.tsx": ` | ||
import Counter from "../components/counter"; | ||
export default function About() { | ||
return ( | ||
<main> | ||
<h1>About Title</h1> | ||
<Counter id="about-counter" /> | ||
</main> | ||
) | ||
} | ||
`, | ||
"app/components/counter.tsx": ` | ||
import * as React from "react"; | ||
export default function Counter({ id }) { | ||
let [count, setCount] = React.useState(0); | ||
return ( | ||
<p> | ||
<button id={id} onClick={() => setCount(count + 1)}>inc {count}</button> | ||
</p> | ||
); | ||
} | ||
`, | ||
}, | ||
}); | ||
|
||
let sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
|
||
let wait = async ( | ||
callback: () => boolean, | ||
{ timeoutMs = 1000, intervalMs = 250 } = {} | ||
) => { | ||
let start = Date.now(); | ||
while (Date.now() - start <= timeoutMs) { | ||
if (callback()) { | ||
return; | ||
} | ||
await sleep(intervalMs); | ||
} | ||
throw Error(`wait: timeout ${timeoutMs}ms`); | ||
}; | ||
|
||
let bufferize = (stream: Readable): (() => string) => { | ||
let buffer = ""; | ||
stream.on("data", (data) => (buffer += data.toString())); | ||
return () => buffer; | ||
}; | ||
|
||
test("HMR", async ({ page }) => { | ||
// uncomment for debugging | ||
// page.on("console", (msg) => console.log(msg.text())); | ||
page.on("pageerror", (err) => console.log(err.message)); | ||
|
||
let appServerPort = await getPort({ port: makeRange(3080, 3089) }); | ||
let port = await getPort({ port: makeRange(3090, 3099) }); | ||
let projectDir = await createFixtureProject(fixture({ port, appServerPort })); | ||
|
||
// spin up dev server | ||
let dev = execa("npm", ["run", "dev:remix"], { cwd: projectDir }); | ||
let devStdout = bufferize(dev.stdout!); | ||
let devStderr = bufferize(dev.stderr!); | ||
await wait( | ||
() => { | ||
let stderr = devStderr(); | ||
if (stderr.length > 0) throw Error(stderr); | ||
return /💿 Built in /.test(devStdout()); | ||
}, | ||
{ timeoutMs: 10_000 } | ||
); | ||
|
||
// spin up app server | ||
let app = execa("npm", ["run", "dev:app"], { cwd: projectDir }); | ||
let appStdout = bufferize(app.stdout!); | ||
let appStderr = bufferize(app.stderr!); | ||
await wait( | ||
() => { | ||
let stderr = appStderr(); | ||
if (stderr.length > 0) throw Error(stderr); | ||
return /✅ app ready: /.test(appStdout()); | ||
}, | ||
{ | ||
timeoutMs: 10_000, | ||
} | ||
); | ||
|
||
try { | ||
await page.goto(`http://localhost:${appServerPort}`, { | ||
waitUntil: "networkidle", | ||
}); | ||
|
||
// `<input />` value as page state that | ||
// would be wiped out by a full page refresh | ||
// but should be persisted by hmr | ||
let input = page.getByLabel("Root Input"); | ||
expect(input).toBeVisible(); | ||
await input.type("asdfasdf"); | ||
|
||
let counter = await page.waitForSelector("#root-counter"); | ||
await counter.click(); | ||
await page.waitForSelector(`#root-counter:has-text("inc 1")`); | ||
|
||
let indexPath = path.join(projectDir, "app", "routes", "index.tsx"); | ||
let originalIndex = fs.readFileSync(indexPath, "utf8"); | ||
let counterPath = path.join(projectDir, "app", "components", "counter.tsx"); | ||
let originalCounter = fs.readFileSync(counterPath, "utf8"); | ||
|
||
// make content and style changed to index route | ||
let newIndex = ` | ||
import { useLoaderData } from "@remix-run/react"; | ||
export default function Index() { | ||
const t = useLoaderData(); | ||
return ( | ||
<main> | ||
<h1 className="text-white bg-black">Changed</h1> | ||
</main> | ||
) | ||
} | ||
`; | ||
fs.writeFileSync(indexPath, newIndex); | ||
|
||
// detect HMR'd content and style changes | ||
await page.waitForLoadState("networkidle"); | ||
let h1 = page.getByText("Changed"); | ||
await h1.waitFor({ timeout: 2000 }); | ||
expect(h1).toHaveCSS("color", "rgb(255, 255, 255)"); | ||
expect(h1).toHaveCSS("background-color", "rgb(0, 0, 0)"); | ||
|
||
// verify that `<input />` value was persisted (i.e. hmr, not full page refresh) | ||
expect(await page.getByLabel("Root Input").inputValue()).toBe("asdfasdf"); | ||
await page.waitForSelector(`#root-counter:has-text("inc 1")`); | ||
|
||
// undo change | ||
fs.writeFileSync(indexPath, originalIndex); | ||
await page.getByText("Index Title").waitFor({ timeout: 2000 }); | ||
expect(await page.getByLabel("Root Input").inputValue()).toBe("asdfasdf"); | ||
await page.waitForSelector(`#root-counter:has-text("inc 1")`); | ||
|
||
// add loader | ||
let withLoader1 = ` | ||
import { json } from "@remix-run/node"; | ||
import { useLoaderData } from "@remix-run/react"; | ||
|
||
export let loader = () => json({ hello: "world" }) | ||
|
||
export default function Index() { | ||
let { hello } = useLoaderData<typeof loader>(); | ||
return ( | ||
<main> | ||
<h1>Hello, {hello}</h1> | ||
</main> | ||
) | ||
} | ||
`; | ||
fs.writeFileSync(indexPath, withLoader1); | ||
await page.getByText("Hello, world").waitFor({ timeout: 2000 }); | ||
expect(await page.getByLabel("Root Input").inputValue()).toBe("asdfasdf"); | ||
await page.waitForSelector(`#root-counter:has-text("inc 1")`); | ||
|
||
let withLoader2 = ` | ||
import { json } from "@remix-run/node"; | ||
import { useLoaderData } from "@remix-run/react"; | ||
|
||
export function loader() { | ||
return json({ hello: "planet" }) | ||
} | ||
|
||
export default function Index() { | ||
let { hello } = useLoaderData<typeof loader>(); | ||
return ( | ||
<main> | ||
<h1>Hello, {hello}</h1> | ||
</main> | ||
) | ||
} | ||
`; | ||
fs.writeFileSync(indexPath, withLoader2); | ||
await page.getByText("Hello, planet").waitFor({ timeout: 2000 }); | ||
expect(await page.getByLabel("Root Input").inputValue()).toBe("asdfasdf"); | ||
await page.waitForSelector(`#root-counter:has-text("inc 1")`); | ||
|
||
// change shared component | ||
let updatedCounter = ` | ||
import * as React from "react"; | ||
export default function Counter({ id }) { | ||
let [count, setCount] = React.useState(0); | ||
return ( | ||
<p> | ||
<button id={id} onClick={() => setCount(count - 1)}>dec {count}</button> | ||
</p> | ||
); | ||
} | ||
`; | ||
fs.writeFileSync(counterPath, updatedCounter); | ||
await page.waitForSelector(`#root-counter:has-text("dec 1")`); | ||
counter = await page.waitForSelector("#root-counter"); | ||
await counter.click(); | ||
await counter.click(); | ||
await page.waitForSelector(`#root-counter:has-text("dec -1")`); | ||
|
||
await page.click(`a[href="/about"]`); | ||
let aboutCounter = await page.waitForSelector( | ||
`#about-counter:has-text("dec 0")` | ||
); | ||
await aboutCounter.click(); | ||
await page.waitForSelector(`#about-counter:has-text("dec -1")`); | ||
|
||
// undo change | ||
fs.writeFileSync(counterPath, originalCounter); | ||
|
||
counter = await page.waitForSelector(`#root-counter:has-text("inc -1")`); | ||
await counter.click(); | ||
counter = await page.waitForSelector(`#root-counter:has-text("inc 0")`); | ||
|
||
aboutCounter = await page.waitForSelector( | ||
`#about-counter:has-text("inc -1")` | ||
); | ||
await aboutCounter.click(); | ||
aboutCounter = await page.waitForSelector( | ||
`#about-counter:has-text("inc 0")` | ||
); | ||
} finally { | ||
dev.kill(); | ||
app.kill(); | ||
console.log(devStderr()); | ||
console.log(appStderr()); | ||
} | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which of these are short-term limitations and which are really hard problems that will have to be solved longer-term (or never solved at all)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👆 short-term, should be resolved before HMR is stabilized
👆 we already prototyped a
import.meta.hot
API so my gut is that we can also do this short-term as needed👆 If we have
import.meta.hot
API, then HMR should be implementable in user-land for any other rendering frameworks/libraries. Out-of-the-box HMR support for other renderers would come after we support that renderer (e.g. after@remix-run/vue
exists or something like that).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I've noticed that HMR can be quite slow (several seconds after save). Is that expected? If you want to play around with a real world project that has this issue, check: https://github.com/epicweb-dev/rocket-rental
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, planning on optimizing for perf soon. Though we are also limited by how quickly the app server can pick up changes. E.g. if using
nodemon
, how quickly can the app server be restarted. I know for Rocket Rental you are usingtsx watch
, so that might be a limiting factor.That said, I think we should be able to make app server restart only blocking for HDR, and not for HMR (e.g. if we can serve the updated assets immediately from the dev server). For making HDR faster, server-side HMR (instead of server restart) would be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think my server is restarting. I'm doing the purge require cache approach instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah whoops. Saw
tsx watch
and made assumptions, but you are only watching for changes in./index.js
and notbuild/index.js
. Make sense.Yea so to summarize, we can optimize HMR speed directly, but have limitations on HDR speed depending on how fast your app server responds with an up-to-date build hash via
__REMIX_ASSETS_MANIFEST
endpoint. Should be able to at least speed up HMR part as part of stabilizing this feature.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great to hear! Thanks Pedro!