Skip to content
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

fix(dev): fix hot-reload with e2e test #1021

Merged
merged 24 commits into from
Nov 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions e2e/fixtures/hot-reload/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"name": "hot-reload",
"version": "0.1.0",
"type": "module",
"private": true,
"scripts": {
"dev": "waku dev",
"build": "waku build",
"start": "waku start"
},
"dependencies": {
"react": "19.0.0-rc-5c56b873-20241107",
"react-dom": "19.0.0-rc-5c56b873-20241107",
"react-server-dom-webpack": "19.0.0-rc-5c56b873-20241107",
"waku": "workspace:*"
},
"devDependencies": {
"@types/react": "^18.3.12",
"@types/react-dom": "^18.3.1",
"typescript": "^5.6.3"
}
}
18 changes: 18 additions & 0 deletions e2e/fixtures/hot-reload/src/components/counter.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use client';

import { useState } from 'react';

export const Counter = () => {
const [count, setCount] = useState(0);
return (
<div
data-testid="counter"
style={{ border: '3px blue dashed', margin: '1em', padding: '1em' }}
>
<p data-testid="count">{count}</p>
<button data-testid="increment" onClick={() => setCount((c) => c + 1)}>
Increment
</button>
</div>
);
};
16 changes: 16 additions & 0 deletions e2e/fixtures/hot-reload/src/pages/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { Counter } from '../components/counter.js';

export default async function HomePage() {
return (
<div>
<p>Home Page</p>
<Counter />
</div>
);
}

export const getConfig = async () => {
return {
render: 'static',
} as const;
};
17 changes: 17 additions & 0 deletions e2e/fixtures/hot-reload/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"compilerOptions": {
"composite": true,
"strict": true,
"target": "esnext",
"downlevelIteration": true,
"esModuleInterop": true,
"module": "nodenext",
"skipLibCheck": true,
"noUncheckedIndexedAccess": true,
"exactOptionalPropertyTypes": true,
"types": ["react/experimental"],
"jsx": "react-jsx",
"rootDir": "./src",
"outDir": "./dist"
}
}
122 changes: 122 additions & 0 deletions e2e/hot-reload.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { expect } from '@playwright/test';
import { execSync, exec } from 'node:child_process';
import { fileURLToPath } from 'node:url';
import { cp, mkdtemp, readFile, writeFile } from 'node:fs/promises';
import { join } from 'node:path';
import { tmpdir } from 'node:os';
import { createRequire } from 'node:module';
import waitPort from 'wait-port';

import {
debugChildProcess,
getFreePort,
isPortAvailable,
terminate,
test,
} from './utils.js';

let standaloneDir: string;
const fixtureDir = fileURLToPath(
new URL('./fixtures/hot-reload', import.meta.url),
);
const wakuDir = fileURLToPath(new URL('../packages/waku', import.meta.url));
const { version } = createRequire(import.meta.url)(
join(wakuDir, 'package.json'),
);

async function run() {
const HMR_PORT = 24678;
if (!(await isPortAvailable(HMR_PORT))) {
if (process.platform === 'win32') {
const output = execSync(
`for /f "tokens=5" %A in ('netstat -ano ^| findstr :${HMR_PORT} ^| findstr LISTENING') do @echo %A`,
{
encoding: 'utf8',
},
);
if (output) {
await terminate(parseInt(output));
}
} else {
const output = execSync(`lsof -i:${HMR_PORT} | awk 'NR==2 {print $2}'`, {
encoding: 'utf8',
});
if (output) {
await terminate(parseInt(output));
}
}
}
const port = await getFreePort();
const cp = exec(
`node ${join(standaloneDir, './node_modules/waku/dist/cli.js')} dev --port ${port}`,
{ cwd: standaloneDir },
);
debugChildProcess(cp, fileURLToPath(import.meta.url), [
/ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time/,
]);
await waitPort({ port });
return [port, cp.pid] as const;
}

async function modifyFile(file: string, search: string, replace: string) {
const content = await readFile(join(standaloneDir, file), 'utf-8');
await writeFile(join(standaloneDir, file), content.replace(search, replace));
}

test.describe('hot reload', () => {
test.beforeEach(async () => {
// GitHub Action on Windows doesn't support mkdtemp on global temp dir,
// Which will cause files in `src` folder to be empty.
// I don't know why
const tmpDir = process.env.TEMP_DIR ? process.env.TEMP_DIR : tmpdir();
standaloneDir = await mkdtemp(join(tmpDir, 'waku-hot-reload-'));
await cp(fixtureDir, standaloneDir, {
filter: (src) => {
return !src.includes('node_modules') && !src.includes('dist');
},
recursive: true,
});
execSync(`pnpm pack --pack-destination ${standaloneDir}`, {
cwd: wakuDir,
stdio: 'inherit',
});
const name = `waku-${version}.tgz`;
execSync(`npm install --force ${join(standaloneDir, name)}`, {
cwd: standaloneDir,
stdio: 'inherit',
});
});

test('simple case', async ({ page }) => {
const [port, pid] = await run();
await page.goto(`http://localhost:${port}/`);
await expect(page.getByText('Home Page')).toBeVisible();
await expect(page.getByTestId('count')).toHaveText('0');
await page.getByTestId('increment').click();
await expect(page.getByTestId('count')).toHaveText('1');
// Server component hot reload
await modifyFile('src/pages/index.tsx', 'Home Page', 'Modified Page');
await expect(page.getByText('Modified Page')).toBeVisible();
await page.waitForTimeout(500); // need to wait not to full reload
await expect(page.getByTestId('count')).toHaveText('1');
await page.getByTestId('increment').click();
await expect(page.getByTestId('count')).toHaveText('2');
// Client component HMR
await modifyFile('src/components/counter.tsx', 'Increment', 'Plus One');
await expect(page.getByText('Plus One')).toBeVisible();
await page.waitForTimeout(500); // need to wait not to full reload
await expect(page.getByTestId('count')).toHaveText('2');
await page.getByTestId('increment').click();
await expect(page.getByTestId('count')).toHaveText('3');
// Server component hot reload again
await modifyFile('src/pages/index.tsx', 'Modified Page', 'Edited Page');
await expect(page.getByText('Edited Page')).toBeVisible();
await page.waitForTimeout(500); // need to wait not to full reload
// FIXME The following should pass but not for now.
// It's probably because Vite adds `?t=...` timestamp with HMR.
// await expect(page.getByTestId('count')).toHaveText('3');
// await page.getByTestId('increment').click();
// await expect(page.getByTestId('count')).toHaveText('4');
await terminate(pid!);
});
});
22 changes: 20 additions & 2 deletions e2e/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,33 @@ const unexpectedErrors: RegExp[] = [
];

export async function getFreePort(): Promise<number> {
return new Promise<number>((res) => {
return new Promise<number>((resolve) => {
const srv = net.createServer();
srv.listen(0, () => {
const port = (srv.address() as net.AddressInfo).port;
srv.close(() => res(port));
srv.close(() => resolve(port));
});
});
}

export async function isPortAvailable(port: number): Promise<boolean> {
return new Promise<boolean>((resolve, reject) => {
const srv = net.createServer();
srv.once('error', (err) => {
if ((err as any).code === 'EADDRINUSE') {
resolve(false);
} else {
reject(err);
}
});
srv.once('listening', () => {
srv.close();
resolve(true);
});
srv.listen(port);
});
}

export function debugChildProcess(
cp: ChildProcess,
sourceFile: string,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ export const fsRouterTypegenPlugin = (opts: { srcDir: string }): Plugin => {
await writeFile(outputFile, formatted, 'utf-8');
};

server.watcher.add(opts.srcDir);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this fixes the issue of failing on windows and ubuntu. @tylersayshi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we did server.watcher.add(pagesDir) instead? .gen.ts will be outside the pages dir so this should be fine I'd think

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can try, but is removing this a problem? It still generates .gen.ts file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea for adding it here is that it will updates pages.gen.ts when new pages are added or removed. Without the watcher I don't think it will be able to update while the dev server is running. Just on startup only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works on mac... I'll watch to see if the other tests pass but I expect them to 🤞

#1023

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it passed 🎉

server.watcher.on('change', async (file) => {
if (!outputFile || outputFile.endsWith(file)) {
return;
Expand Down
1 change: 1 addition & 0 deletions packages/waku/src/lib/plugins/vite-plugin-rsc-delegate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export function rscDelegatePlugin(
callback({ type: 'custom', event: 'rsc-reload' });
}
}
return [];
},
async transform(code, id) {
// id can contain query string with vite deps optimization
Expand Down
27 changes: 16 additions & 11 deletions packages/waku/src/lib/plugins/vite-plugin-rsc-hmr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ if (import.meta.hot && !globalThis.__WAKU_HMR_CONFIGURED__) {
`;

export function rscHmrPlugin(): Plugin {
const wakuClientDist = decodeFilePathFromAbsolute(
joinPath(fileURLToFilePath(import.meta.url), '../../../client.js'),
const wakuMinimalClientDist = decodeFilePathFromAbsolute(
joinPath(fileURLToFilePath(import.meta.url), '../../../minimal/client.js'),
);
const wakuRouterClientDist = decodeFilePathFromAbsolute(
joinPath(fileURLToFilePath(import.meta.url), '../../../router/client.js'),
Expand All @@ -75,7 +75,7 @@ export function rscHmrPlugin(): Plugin {
];
},
async transform(code, id) {
if (id.startsWith(wakuClientDist)) {
if (id.startsWith(wakuMinimalClientDist)) {
// FIXME this is fragile. Can we do it better?
return code.replace(
/\nexport const fetchRsc = \(.*?\)=>\{/,
Expand All @@ -101,15 +101,16 @@ export function rscHmrPlugin(): Plugin {
);
} else if (id.startsWith(wakuRouterClientDist)) {
// FIXME this is fragile. Can we do it better?
const INNER_ROUTER_LINE = 'function InnerRouter() {';
return code.replace(
INNER_ROUTER_LINE,
INNER_ROUTER_LINE +
/\nconst InnerRouter = \(.*?\)=>\{/,
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to change this to NewInnerRouter for #1003.

(m) =>
m +
`
{
const refetchRoute = () => {
const rscPath = getInputString(loc.path);
refetch(rscPath, loc.searchParams);
const rscPath = encodeRoutePath(route.path);
const rscParams = createRscParams(route.query, []);
refetch(rscPath, rscParams);
};
globalThis.__WAKU_RSC_RELOAD_LISTENERS__ ||= [];
const index = globalThis.__WAKU_RSC_RELOAD_LISTENERS__.indexOf(globalThis.__WAKU_REFETCH_ROUTE__);
Expand All @@ -125,13 +126,17 @@ export function rscHmrPlugin(): Plugin {
}
},
handleHotUpdate({ file }) {
if (file.endsWith('/pages.gen.ts')) {
// auto generated file by fsRouterTypegenPlugin
return [];
}
const moduleLoading = (globalThis as any).__WAKU_CLIENT_MODULE_LOADING__;
const moduleCache = (globalThis as any).__WAKU_CLIENT_MODULE_CACHE__;
if (!moduleLoading || !moduleCache) {
return;
}
if (file.startsWith(viteServer.config.root)) {
file = file.slice(viteServer.config.root.length);
if (file.startsWith(viteServer.config.root + '/')) {
file = file.slice(viteServer.config.root.length + 1);
}
const id = filePathToFileURL(file);
if (moduleLoading.has(id)) {
Expand All @@ -149,7 +154,7 @@ export function rscHmrPlugin(): Plugin {

const pendingMap = new WeakMap<ReturnType<typeof viteHot>, Set<string>>();

export function viteHot(viteServer: ViteDevServer): HMRBroadcaster {
function viteHot(viteServer: ViteDevServer): HMRBroadcaster {
return viteServer.hot ?? viteServer.ws;
}

Expand Down
25 changes: 25 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tsconfig.e2e.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@
},
{
"path": "./e2e/fixtures/broken-links/tsconfig.json"
},
{
"path": "./e2e/fixtures/hot-reload/tsconfig.json"
}
]
}
Loading