feat: Support SSR, SSG and HMR#10
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Harper-Vite integration, introducing a hybrid production mode that statically serves production builds and supports automatic recompilation alongside the existing development mode. It also adds support for Server-Side Rendering (SSR), coordinates multi-threaded builds using a file-system lock, and introduces comprehensive tests. The review feedback identifies several critical issues: a race condition in the build lock release check, a severe memory leak in registerHttp due to unresolved promises on successful middleware responses, a downtime window during SSR rebuilds caused by hardcoded directory emptying, performance bottlenecks from synchronous file operations in HTTP handlers, and a potential double-close bug in registerShutdown.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function registerHttp(scope: any, middleware: Middleware): void { | ||
| if (!scope?.server?.http) return; | ||
| scope.server.http( | ||
| (request: any, nextLayer: (request: any) => unknown) => | ||
| new Promise((resolve, reject) => { | ||
| middleware(request._nodeRequest, request._nodeResponse, (err?: unknown) => { | ||
| if (err) return reject(err); | ||
| try { | ||
| resolve(nextLayer(request)); | ||
| } catch (e) { | ||
| reject(e); | ||
| } | ||
| }); | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
In registerHttp, the returned Promise is only resolved when the middleware calls next(). However, Connect-style middlewares (such as sirv or Vite's middleware) do not call next() when they successfully handle a request and send a response. This causes the Promise to remain pending forever for almost all static asset and HTML requests, leading to a severe memory leak under production load.
We should listen to the response's finish and close events to resolve the Promise and clean up listeners when the response is completed.
export function registerHttp(scope: any, middleware: Middleware): void {
if (!scope?.server?.http) return;
scope.server.http(
(request: any, nextLayer: (request: any) => unknown) =>
new Promise((resolve, reject) => {
const res = request._nodeResponse;
const cleanup = () => {
res.removeListener('finish', onFinished);
res.removeListener('close', onClosed);
};
const onFinished = () => {
cleanup();
resolve(undefined);
};
const onClosed = () => {
cleanup();
resolve(undefined);
};
res.on('finish', onFinished);
res.on('close', onClosed);
middleware(request._nodeRequest, res, (err?: unknown) => {
cleanup();
if (err) return reject(err);
try {
resolve(nextLayer(request));
} catch (e) {
reject(e);
}
});
})
);
}| if (ssrEntry) { | ||
| await viteWrapper.build({ | ||
| root, | ||
| build: { ssr: ssrEntry, outDir: serverOutDir, emptyOutDir: true }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The SSR build has emptyOutDir: true hardcoded. This means every automatic rebuild will delete the entire serverOutDir before compilation starts. During this compilation window (which can take several seconds), any incoming SSR request will fail with ENOENT or module resolution errors because the server entry file is missing. This introduces a downtime window, contradicting the goal of a zero-downtime atomic swap.
To achieve a truly zero-downtime swap, consider building to a temporary directory and renaming it upon success, or at least set emptyOutDir: false to avoid deleting the existing bundle before the new one is ready.
| if (ssrEntry) { | |
| await viteWrapper.build({ | |
| root, | |
| build: { ssr: ssrEntry, outDir: serverOutDir, emptyOutDir: true }, | |
| }); | |
| } | |
| if (ssrEntry) { | |
| await viteWrapper.build({ | |
| root, | |
| build: { ssr: ssrEntry, outDir: serverOutDir, emptyOutDir: false }, | |
| }); | |
| } |
| const template = readFileSync(join(clientOutDir, 'index.html'), 'utf-8'); | ||
| // Cache-bust the import by the bundle's mtime so every worker picks up a rebuilt SSR bundle, | ||
| // regardless of which worker performed the build. | ||
| const version = statSync(serverEntryPath).mtimeMs; |
There was a problem hiding this comment.
Using synchronous file system operations (readFileSync and statSync) inside the HTTP request handler blocks the single-threaded Node.js event loop on every single HTML request. Under production load, this will severely limit the throughput of the server and increase response latency.
Consider using asynchronous file system operations (fs.promises.readFile and fs.promises.stat) or, even better, caching the index.html template in memory since it only changes when a rebuild completes.
| export function registerShutdown(scope: any, close: () => unknown): void { | ||
| const shutdownHandler = (msg: any) => { | ||
| if (msg?.type === 'shutdown') close(); | ||
| }; | ||
|
|
||
| scope.on('close', () => { | ||
| close(); | ||
| parentPort?.off('message', shutdownHandler); | ||
| }); | ||
|
|
||
| parentPort?.on('message', shutdownHandler); | ||
| } |
There was a problem hiding this comment.
In registerShutdown, if a shutdown message is received via parentPort, close() is called. However, if the scope later emits close, close() will be called a second time. To prevent potential errors or warnings from double-closing resources (like the Vite server), we should ensure close() is only executed once.
| export function registerShutdown(scope: any, close: () => unknown): void { | |
| const shutdownHandler = (msg: any) => { | |
| if (msg?.type === 'shutdown') close(); | |
| }; | |
| scope.on('close', () => { | |
| close(); | |
| parentPort?.off('message', shutdownHandler); | |
| }); | |
| parentPort?.on('message', shutdownHandler); | |
| } | |
| export function registerShutdown(scope: any, close: () => unknown): void { | |
| let closed = false; | |
| const safeClose = () => { | |
| if (closed) return; | |
| closed = true; | |
| close(); | |
| }; | |
| const shutdownHandler = (msg: any) => { | |
| if (msg?.type === 'shutdown') safeClose(); | |
| }; | |
| scope.on('close', () => { | |
| safeClose(); | |
| parentPort?.off('message', shutdownHandler); | |
| }); | |
| parentPort?.on('message', shutdownHandler); | |
| } |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
I like the idea, but the implementation needs some work. I assume an agent created this. Could you potentially have it review the Next.js plugin so it can see some better development patterns?
| import { dirname, join } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| const root = join(dirname(fileURLToPath(import.meta.url)), '..'); |
There was a problem hiding this comment.
| import { dirname, join } from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| const root = join(dirname(fileURLToPath(import.meta.url)), '..'); | |
| import { join } from 'node:path'; | |
| const root = join(import.meta.dirname, '..'); |
|
@Ethan-Arrowood this is a WIP that i've had going for a month or so, first hand crafted, and then gaining momentum today with Claude. I wanted to get it in front of you and Austin as it's one of the biggest hurdles to people building useful things inside Studio from their browser. Adapting the nextjs examples into create-harper would be an additional win that we could start doing there too, but I'm learning. |
|
Yes, I totally agree this plugin is important |
This adds support for this plugin to run in production, with or without SSR enabled too. This can let us run Vite based apps in Studio, in theory.
I created new templates in create-harper over at HarperFast/create-harper#98