Feat: integrate SSR toolkit workflow with module-runner runtime and Vue SSR examples - #2277
Feat: integrate SSR toolkit workflow with module-runner runtime and Vue SSR examples#2277NidMo wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughIntroduces a comprehensive module runner system for server-side module evaluation, extensive SSR framework with CLI commands (dev/build/preview), module fetch capabilities with transform caching, and safety improvements to module update path handling with sanitization and guards. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Importer
participant Runner as FarmModuleRunner
participant Transport as ModuleRunnerTransport
participant Evaluator as ModuleEvaluator
participant Cache as EvaluatedModules
Client->>Runner: import(url)
activate Runner
Runner->>Cache: getModuleById(url)
Cache-->>Runner: module | undefined
alt Module cached
Runner-->>Client: return cached exports
else Module not cached
Runner->>Transport: invoke('fetchModule', [id, importer])
activate Transport
Transport-->>Runner: FetchResult
deactivate Transport
alt Cached fetch result
Runner-->>Client: return cached
else External fetch result
Runner->>Evaluator: runExternalModule(file, type)
Evaluator-->>Runner: module value
Runner-->>Client: return value
else Inlined fetch result
Runner->>Cache: ensureModule(id, url)
Cache-->>Runner: EvaluatedModuleNode
Runner->>Evaluator: runInlinedModule(context, code, module)
activate Evaluator
Evaluator->>Evaluator: evaluate code with __farm_ssr_*
Evaluator-->>Runner: void (exports in context)
deactivate Evaluator
Runner->>Cache: cache exports
Runner-->>Client: return exports
end
end
deactivate Runner
sequenceDiagram
participant User as User
participant CLI as CLI
participant ConfigResolver as Config Resolver
participant DevServer as SSR Dev Server
participant FarmServer as Farm Server
participant ModuleRunner as Module Runner
participant Template as Template Engine
User->>CLI: farm ssr dev
activate CLI
CLI->>ConfigResolver: resolveSsrRunOptions(options)
ConfigResolver-->>CLI: SsrRunOptions (client/server config)
CLI->>DevServer: createSsrDevServer(options)
activate DevServer
DevServer->>FarmServer: createFarmServer(config)
activate FarmServer
FarmServer-->>DevServer: farm server instance
deactivate FarmServer
DevServer->>ModuleRunner: createModuleRunner()
activate ModuleRunner
ModuleRunner-->>DevServer: module runner instance
deactivate ModuleRunner
DevServer-->>CLI: dev server with render/listen
deactivate DevServer
CLI->>DevServer: listen(port)
activate DevServer
Note over DevServer: Server listening
deactivate DevServer
User->>DevServer: GET /
activate DevServer
DevServer->>Template: loadTemplate(options.ssr.template)
Template-->>DevServer: template content
DevServer->>ModuleRunner: import(ssr.entry)
activate ModuleRunner
ModuleRunner-->>DevServer: ssr module
deactivate ModuleRunner
DevServer->>DevServer: ssr.render(url)
Note over DevServer: Render app HTML
DevServer->>Template: injectAppHtml(template, html)
Template-->>DevServer: final HTML
DevServer-->>User: 200 text/html
deactivate DevServer
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (21)
examples/ssr-toolkit/README.md-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorLink the known
compilation.minify=falseissue to its tracker entry.The note documents a known Farm core bug but provides no reference, making it hard for users (or future contributors fixing it) to track it.
📝 Suggested revision
-当前 example 为保证 preview 懒加载路由稳定，client 构建使用了 `compilation.minify=false`（已记录为 Farm 核心已知问题，后续可在核心修复后恢复）。 +当前 example 为保证 preview 懒加载路由稳定，client 构建使用了 `compilation.minify=false`（已记录为 Farm 核心已知问题，见 issue `#XXXX`，后续可在核心修复后恢复）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/README.md` at line 16, Update the README note that mentions compilation.minify=false to include a direct link to the Farm core bug tracker entry (issue ID/URL) and a one-line summary of the bug; locate the text containing "compilation.minify=false" in examples/ssr-toolkit/README.md and append the tracker URL and issue number (or repo/issue reference) so future readers and contributors can quickly find the canonical bug report.crates/node/src/lib.rs-965-971 (1)
965-971:⚠️ Potential issue | 🟡 Minor
_optionsparameter is entirely unused —cachedandstartOffsetare silently ignored.The
_optionsparameter (prefixed with_to suppress warnings) is never consumed. Callers passing{ cached: true }or{ startOffset: N }will get no behavioral difference. If caching opt-in or offset-based source maps are part of the planned API contract, this should at minimum log a warning or be documented as not-yet-implemented.Would you like me to open an issue to track implementing the
cachedandstartOffsetoption handling?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/lib.rs` around lines 965 - 971, The fetch_module method currently ignores its _options parameter so callers' cached and startOffset flags have no effect; update fetch_module to parse the JsFetchModuleOptions (remove the leading underscore from the parameter name to use it) and handle cached by consulting/returning from whatever module cache logic is used in this crate, and handle startOffset by adjusting the returned source or source-map offsets (or applying the offset when creating JsFetchModuleResult); if full support isn't yet implemented, at minimum emit a debug/warn log inside fetch_module when cached or startOffset are provided to make the limitation visible to callers and consider creating an issue to track full implementation.packages/core/src/module-runner/createImportMeta.ts-93-105 (1)
93-105:⚠️ Potential issue | 🟡 MinorSame
encodeURIconcern intoAbsoluteDirectoryFileUrl.Same issue as
toFileUrl—#and?in directory paths will produce broken file URLs. If you address it intoFileUrl, apply the same fix here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/module-runner/createImportMeta.ts` around lines 93 - 105, The toAbsoluteDirectoryFileUrl function incorrectly uses encodeURI which doesn't escape '#' and '?' making file URLs break; update to use the same fix applied to toFileUrl: build the file URL by encoding each path segment (e.g., split normalized path by '/', encodeURIComponent each segment) and then rejoin with '/' and append the trailing slash, or construct the URL via the URL constructor with properly encoded segments, ensuring drive-letter and absolute-path branches (the branches in toAbsoluteDirectoryFileUrl) produce correctly encoded file:// URLs; mirror the exact approach you implemented in toFileUrl so both functions behave consistently.packages/core/src/module-runner/serverTransport.ts-48-53 (1)
48-53:⚠️ Potential issue | 🟡 MinorMissing guard for unknown invoke handler name.
If
deps.invokeHandlers[name]isundefined, the cast to a function and the subsequent call will throw an unhelpfulTypeError: handler is not a function. Consider adding a descriptive check.Proposed fix
async invoke(name, data) { const handler = deps.invokeHandlers[name] as ( ...args: unknown[] ) => Promise<unknown>; + if (typeof handler !== 'function') { + throw new Error( + `[farm module runner] Invoke handler "${String(name)}" is not registered.` + ); + } return (await handler(...(data as unknown[]))) as never; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/module-runner/serverTransport.ts` around lines 48 - 53, The invoke method casts and calls deps.invokeHandlers[name] without checking existence, which yields an unhelpful TypeError when the handler is missing; update the invoke function to verify that deps.invokeHandlers[name] exists and is a function before calling it (e.g., check typeof handler === "function"), and if not, throw a descriptive error (for example "Unknown invoke handler: <name>" or similar) so callers get a clear message; reference the invoke method and deps.invokeHandlers to locate where to add this guard and error.packages/core/src/module-runner/runner.ts-64-67 (1)
64-67:⚠️ Potential issue | 🟡 Minor
import()does not checkthis.closed, allowing use-after-close.After
close()is called, the transport is disconnected and caches are cleared. Callingimport()would attempt to invoke on a disconnected transport, producing confusing errors. Consider adding a guard.Proposed fix
async import<T = unknown>(url: string): Promise<T> { + if (this.closed) { + throw new Error( + '[farm module runner] Cannot import after runner has been closed.' + ); + } const mod = await this.cachedModule(url); return (await this.cachedRequest(url, mod)) as T; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/module-runner/runner.ts` around lines 64 - 67, The import<T>(url: string) method calls this.cachedModule and this.cachedRequest even after close() has been called, which can trigger operations on a disconnected transport; add a guard at the start of import (check this.closed) and throw a clear error (or return a rejected Promise) if closed is true, preventing use-after-close; ensure the same pattern is used for related entry points that call cachedModule/cachedRequest so callers get a consistent "runner is closed" error instead of transport errors.packages/core/src/module-runner/runner.ts-55-61 (1)
55-61:⚠️ Potential issue | 🟡 MinorUnhandled rejection risk if
transport.connectis asynchronous and fails.
voiddiscards the return value. If a custom transport'sconnect()returns a rejectingPromise, the rejection is never caught, potentially crashing the process with an unhandled promise rejection.Proposed fix
- void this.options.transport.connect({ + Promise.resolve(this.options.transport.connect({ onMessage: (payload) => this.handleHotPayload(payload), onDisconnection: () => { // No-op for now. Keep room for future reconnect strategy. } - }); + })).catch((err) => { + console.warn( + `[farm module runner] HMR transport connect failed: ${err}` + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/module-runner/runner.ts` around lines 55 - 61, The code currently calls this.options.transport.connect(...) with a leading void which discards a possibly rejecting Promise; if connect() is async and rejects it will become an unhandled rejection. Fix by handling the returned Promise from this.options.transport.connect: either await it inside an async function (e.g., make the enclosing method async and use await this.options.transport.connect(...)) or append a .catch(...) to the call to log/handle errors (use your module runner's logger or a provided error handler) so rejections are observed; reference this.options.transport.connect and ensure any cleanup/reconnect logic (onDisconnection/handleHotPayload) remains intact.packages/core/src/server/index.ts-484-500 (1)
484-500:⚠️ Potential issue | 🟡 MinorPotential double-compilation race in
createModuleRunnerif called concurrently.Two concurrent
createModuleRunner()calls can both observe!this.compiler === truebefore either completes#startCompile(), resulting in two compilers being created (the second overwriting the first). While unlikely in current usage, this could cause subtle bugs if the API is used more broadly.A simple guard would prevent re-entrant compilation:
Proposed fix — guard against concurrent compilation
+ private _startCompilePromise: Promise<void> | null = null; + async createModuleRunner( options: Omit<FarmModuleRunnerOptions, 'transport'> = {} ): Promise<FarmModuleRunner> { if (!this.compiler) { - await this.#startCompile(); - this.watcher?.watchExtraFiles?.(); + if (!this._startCompilePromise) { + this._startCompilePromise = this.#startCompile().then(() => { + this.watcher?.watchExtraFiles?.(); + }); + } + await this._startCompilePromise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/index.ts` around lines 484 - 500, createModuleRunner can race when called concurrently because both callers may see !this.compiler and call `#startCompile`; add a single-entry guard (e.g., a private Promise/flag like this._compilePromise or this._isCompiling) so the first caller sets the guard then awaits this.#startCompile(), and subsequent callers await the same promise instead of calling `#startCompile` again; ensure the watcher?.watchExtraFiles() invocation happens after the compile completes and clear the guard when done.packages/core/src/module-runner/createImportMeta.ts-31-67 (1)
31-67:⚠️ Potential issue | 🟡 MinorUsing
encodeURIleaves#and?unencoded, breaking file URLs with these characters in their paths.
encodeURIintentionally leaves#and?unencoded since they're URL syntax delimiters. A file path like/home/user/my#project/file.tswould producefile:///home/user/my#project/file.ts, where everything after#is interpreted as a URL fragment, corrupting the file path.However, importing Node's
pathToFileURLwould break the runtime-agnostic design of this file—it defensively checks forglobalThis.processand is used in non-Node environments. Instead, encode path segments individually usingencodeURIComponent, which properly escapes#,?, and other reserved characters:function toFileUrl(modulePath: string): string { const normalized = modulePath.replace(/\\/g, '/'); if (/^[a-zA-Z]:\//.test(normalized)) { - return `file:///${encodeURI(normalized)}`; + return `file:///${normalized.split('/').map(encodeURIComponent).join('/')}`; } if (/^[a-zA-Z][a-zA-Z\d+\-.]*:/.test(modulePath)) { return modulePath; } if (normalized.startsWith('//')) { - return `file:${encodeURI(normalized)}`; + return `file:${normalized.split('/').map(encodeURIComponent).join('/')}`; } if (normalized.startsWith('/')) { - return `file://${encodeURI(normalized)}`; + return `file://${normalized.split('/').map(encodeURIComponent).join('/')}`; }Apply the same fix to
toAbsoluteDirectoryFileUrl(lines 97, 101).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/module-runner/createImportMeta.ts` around lines 31 - 67, The toFileUrl function currently uses encodeURI(normalized), which leaves characters like '#' and '?' unescaped and breaks file URLs; fix by encoding each path segment with encodeURIComponent instead of encodeURI (split the normalized path on '/', encodeURIComponent for each non-empty segment, then rejoin, preserving leading '//' or '/' and drive-letter windows patterns), and replace all other uses of encodeURI in the file—specifically update toAbsoluteDirectoryFileUrl similarly (encode each path segment with encodeURIComponent when building the file:// URL) so reserved characters are correctly escaped while keeping the existing runtime-agnostic checks and behavior.examples/ssr-toolkit/tsconfig.json-8-8 (2)
8-8:⚠️ Potential issue | 🟡 Minor
"jsx": "react"is wrong for a Vue 3 project — use"preserve"with"jsxImportSource": "vue".If you intend to use TSX with Vue, set
compilerOptions.jsxto"preserve", and setcompilerOptions.jsxImportSourceto"vue". The Vue official base tsconfig (vuejs/tsconfig) also marks"jsx": "preserve"and"jsxImportSource": "vue"as "Required in Vue projects". Using"react"makes TypeScript emitReact.createElementcalls, which produces broken output for any.tsxfile insrc/.🔧 Proposed fix
- "jsx": "react" + "jsx": "preserve", + "jsxImportSource": "vue"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/tsconfig.json` at line 8, The tsconfig currently sets compilerOptions.jsx to "react", which is incorrect for a Vue 3 project; change compilerOptions.jsx to "preserve" and add compilerOptions.jsxImportSource set to "vue" so TypeScript emits Vue-compatible JSX/TSX output. Update the JSON keys compilerOptions.jsx and compilerOptions.jsxImportSource (and ensure any TSX usage relies on these settings) to match the Vue recommended base tsconfig.
8-8:⚠️ Potential issue | 🟡 MinorChange
"jsx": "react"to"preserve"for Vue 3 compatibility.The
"jsx": "react"setting makes TypeScript emitReact.createElementcalls for any.tsx/ JSX syntax. Since this is a Vue 3 project using@vitejs/plugin-vue, the correct value is"preserve", which leaves JSX untouched so that Vite's Vue JSX plugin can handle the transform. With"react", any TSX files will produce broken output at runtime.🔧 Proposed fix
- "jsx": "react" + "jsx": "preserve"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/tsconfig.json` at line 8, The tsconfig "jsx" option is set to "react" which causes TypeScript to emit React.createElement and breaks Vue 3/Vite handling; update the "jsx" compiler option in tsconfig.json from "react" to "preserve" so JSX/TSX is left untouched for `@vitejs/plugin-vue/vue` JSX transform (look for the "jsx" entry in tsconfig.json and change its value).packages/cli/src/utils.ts-58-63 (2)
58-63:⚠️ Potential issue | 🟡 MinorInner
catchsilently swallows the fallback-path error.The inner
catch {}has no binding, soerr.stackon line 60 reports the primary import failure (@farmfe/ssrnot found), not the fallback path failure. If the fallback itself fails for a different reason (e.g., the monorepodist/hasn't been built yet), that root cause is lost. Consider capturing both errors:🔧 Suggested improvement
- } catch { + } catch (fallbackErr) { logger.error( - `Cannot find `@farmfe/ssr` module, Did you successfully install: \n${err.stack},`, + `Cannot find `@farmfe/ssr` module. Primary error: ${err.stack}\nFallback error: ${(fallbackErr as Error).stack}`, { exit: true } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils.ts` around lines 58 - 63, The inner catch currently swallows the fallback error and still logs only the original import error (err.stack); change the empty inner catch to capture the fallback exception (e.g., catch (fallbackErr)) and update the logger.error call (logger.error(...)) to include both the original import error and the fallback error (include err.stack and fallbackErr.stack or their messages) so you surface both failure causes when attempting to require '@farmfe/ssr' and the fallback path.
58-63:⚠️ Potential issue | 🟡 MinorInner
catchswallows the fallback-path error; only the primary error is surfaced.The inner
catch {}has no binding, soerr.stackon line 60 always reports the primary import failure (@farmfe/ssrnot found). If the fallback itself fails for a distinct reason (e.g.,packages/ssr/dist/hasn't been built), that root cause is silently discarded, making debugging very hard.🔧 Suggested fix
- } catch { + } catch (fallbackErr) { logger.error( - `Cannot find `@farmfe/ssr` module, Did you successfully install: \n${err.stack},`, + `Cannot find `@farmfe/ssr` module.\nPrimary error: ${err.stack}\nFallback error: ${(fallbackErr as Error).stack}`, { exit: true } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils.ts` around lines 58 - 63, The inner empty catch is losing the real fallback error; update the inner catch to capture the fallback exception (e.g., catch (fallbackErr)) and include its details when calling logger.error so both the primary import error (err) and the fallback error (fallbackErr) are surfaced; specifically modify the try/catch around the `@farmfe/ssr` import so the logger.error call references err.stack and fallbackErr.stack (or otherwise concatenates both error messages) instead of relying only on err.stack, ensuring you use distinct variable names to avoid shadowing.examples/ssr-toolkit/package.json-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor
@vitejs/plugin-vuebelongs indevDependencies.It's a build-time transform plugin (used only in the Farm client config), not a runtime dependency. Only
vueandvue-routerare genuinely needed at runtime.🔧 Proposed fix
"dependencies": { - "@vitejs/plugin-vue": "^6.0.4", "vue": "^3.3.12", "vue-router": "^4.2.1" }, "devDependencies": { + "@vitejs/plugin-vue": "^6.0.4", "@farmfe/core": "workspace:*",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/package.json` at line 7, Move "@vitejs/plugin-vue" out of the package.json "dependencies" section and add it under "devDependencies" instead; update the package.json so only runtime packages (e.g., "vue", "vue-router") remain in "dependencies" and build-time plugins like "@vitejs/plugin-vue" are declared in "devDependencies" to reflect that the plugin is used only in the Farm client build/config.examples/ssr-toolkit/src/styles/app.less-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorAdd
customSyntaxoverride for LESS files to prevent false Stylelint errors.The project's Stylelint config extends
stylelint-config-standard-scssglobally, which applies SCSS parsing rules to LESS files. This causes the LESS variable syntax (@var: value) to be flagged as unknown at-rules. The fix is to add acustomSyntaxoverride for*.lessfiles in the Stylelint config:🛠️ Proposed Stylelint config override
// .stylelintrc.json { "extends": [ "stylelint-config-standard-scss" ], + "overrides": [ + { + "files": ["**/*.less"], + "customSyntax": "postcss-less" + } + ], "rules": { ... } }Note: Ensure
postcss-lessis installed as a dev dependency for this to work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/src/styles/app.less` around lines 1 - 4, Add a Stylelint override that sets customSyntax: "postcss-less" for "*.less" files (so LESS variable syntax like `@var`: value isn't parsed as SCSS at-rules); update the Stylelint config (e.g., stylelint.config.js or .stylelintrc) to include an overrides entry targeting "*.less" with customSyntax set to "postcss-less", and install postcss-less as a devDependency so the parser is available.packages/cli/src/ssr.ts-53-66 (1)
53-66:⚠️ Potential issue | 🟡 Minor
parsePortaccepts fractional port numbers.
parsePortvalidates that the value is a finite positive number but doesn't reject fractional values like3.5or80.1, which are not valid port numbers.Proposed fix
const parsed = Number(rawPort); - if (!Number.isFinite(parsed) || parsed <= 0) { + if (!Number.isFinite(parsed) || parsed <= 0 || !Number.isInteger(parsed)) { throw new Error( - `[farm ssr] invalid --port value "${String(rawPort)}", expected a positive number.` + `[farm ssr] invalid --port value "${String(rawPort)}", expected a positive integer.` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ssr.ts` around lines 53 - 66, parsePort currently accepts fractional port numbers because it only checks Number.isFinite and >0; update parsePort to also require an integer (e.g., using Number.isInteger(parsed)) so values like 3.5 or 80.1 are rejected and still throw the same error message. Locate the parsePort function and add an integer check alongside the existing finite/positive checks to ensure only whole-number ports are accepted.packages/cli/src/index.ts-239-253 (1)
239-253:⚠️ Potential issue | 🟡 MinorLoose typing at the
resolveSsr()→runSsrCommandboundary creates undetected shape mismatches.
resolveSsr()returnsrunSsrCommandtyped as(options: Record<string, unknown>) => Promise<unknown>, while the actual function expectsSsrRunCommandOptions. TheSsrRunOptionsproduced by the CLI differs in shape from what the SSR package expects:
- CLI's
serverfield is required in all commands, butSsrDevServerOptions.serveris optional- CLI's nested
ssr.templateis optional, butSsrRenderOptions.templateis required- CLI's
ssrobject lacks fields likeshouldHandle,render, andonErrorthatSsrRenderOptionsdefinesThese incompatibilities won't be caught at compile time due to the loose boundary typing. Consider either:
- Using proper return type for
resolveSsr()to enable TypeScript checking, or- Adding runtime validation to catch shape mismatches early
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/index.ts` around lines 239 - 253, The createSsrAction wrapper is calling runSsrCommand from resolveSsr() which is currently typed loosely; tighten the boundary by updating resolveSsr() to return a properly typed signature (e.g., runSsrCommand: (opts: SsrRunCommandOptions) => Promise<void>) or cast/validate resolved value before use, and adapt resolveSsrRunOptions output to match SsrRunCommandOptions (ensure server optionality, required ssr.render/template/shouldHandle/onError fields are present or supply defaults). Specifically, change the resolveSsr() return type or add a runtime validation/mapping step in createSsrAction (between resolveSsrRunOptions and runSsrCommand) to transform/validate the CLI SsrRunOptions into the expected SsrRunCommandOptions shape so compile-time or early runtime errors surface.examples/middleware-mode/src/main.ts-16-22 (1)
16-22:⚠️ Potential issue | 🟡 MinorAsync click handler swallows errors silently.
If
fetchthrows (e.g., network error) orresponse.json()rejects (non-JSON body), the rejection is unhandled: the#runner-resultelement never updates and the browser console receives an unhandled rejection warning.Note: the ast-grep XSS warning for
innerHTMLon lines 3–10 is a false positive — the content is a fully static template literal with no dynamic data.🔧 Suggested fix
button?.addEventListener('click', async () => { - const response = await fetch('/api/runner'); - const payload = await response.json(); - if (result) { - result.textContent = JSON.stringify(payload, null, 2); - } + try { + const response = await fetch('/api/runner'); + const payload = await response.json(); + if (result) { + result.textContent = JSON.stringify(payload, null, 2); + } + } catch (err) { + if (result) { + result.textContent = `Error: ${err instanceof Error ? err.message : String(err)}`; + } + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/middleware-mode/src/main.ts` around lines 16 - 22, The click handler on button (button?.addEventListener('click', async () => { ... })) can throw during fetch() or response.json(), leaving the `#runner-result` element unchanged and causing unhandled rejections; wrap the async handler body in a try/catch, await fetch('/api/runner') and response.json() inside the try, and in the catch set result.textContent to a clear error message (and optionally process/log the error via console.error) so failures are displayed instead of swallowed; ensure you reference the same result variable used to update the DOM.examples/middleware-mode/server.mjs-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorHardcoded HMR port
9801in log may not match actual server config.The HMR port is determined by
farm.config.ts(and the{ hmr: true }option passed tocreateModuleRunner), not hard-coded to9801. This log line can silently mislead developers when the config uses a different port.🔧 Suggested fix
Derive the port from the server config or omit the specific number:
- console.log('hmr ws port: 9801'); + // console.log(`hmr ws port: ${farmServer.config?.server?.hmr?.port ?? '<from config>'}`);Or, if
farmServerexposes the resolved HMR port after startup, log it there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/middleware-mode/server.mjs` at line 37, Remove the hardcoded console.log('hmr ws port: 9801') and instead log the actual HMR port from the runtime config: obtain the HMR port from the farm config or from the server instance (e.g., the createModuleRunner/farmServer result) and use that value in the message (or log a generic "HMR websocket listening" message if the port isn't exposed); update the console.log call in server.mjs accordingly to reference the resolved port variable or omit the numeric port.examples/ssr-toolkit/server/ports.mjs-35-39 (1)
35-39:⚠️ Potential issue | 🟡 MinorError message references
process.envdirectly instead of theexplicitPortvalue.Line 38 uses
process.env[envName]to display the invalid value, but in test contexts the actualexplicitPortcomes from a custom env object (viaparseOptionalNumberinruntime-config.mjs), notprocess.env. The error message would showundefinedin tests rather than the offending value.Consider using the
explicitPortparameter directly:🔧 Proposed fix
if (!isPositiveInteger(explicitPort)) { throw new Error( - `[ssr-toolkit] invalid ${envName}="${process.env[envName]}", expected a positive integer.` + `[ssr-toolkit] invalid ${envName}="${explicitPort}", expected a positive integer.` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/server/ports.mjs` around lines 35 - 39, The error message in the port validation uses process.env[envName] which can be undefined in test contexts where the port comes from parseOptionalNumber/runtime-config.mjs; update the throw in the explicitPort != null branch to reference the explicitPort variable (and envName for context) so the logged invalid value is the actual offending value (e.g., use explicitPort in the message near isPositiveInteger, keeping envName for clarity).examples/ssr-toolkit/server/template.mjs-26-32 (1)
26-32:⚠️ Potential issue | 🟡 MinorLazy
*?truncates at the first nested</div>, leaving a dangling close tag.For templates where the root container has any nested
<div>element (e.g., a loading skeleton or Farm's own documented placeholder pattern<div id="root"><div>app-html-to-replace</div></div>), the regex stops at the inner</div>, producing:<!-- input --> <div id="root"><div>spinner</div></div> <!-- output (broken) --> <div id="root"><!-- appHtml --></div></div>A depth-counting approach fixes this reliably:
🐛 Proposed fix — depth-aware root container replacement
export function injectAppHtmlIntoBuiltTemplate(template, appHtml) { - const rootContainerPattern = /<div\s+id=(?:"root"|'root'|root)[^>]*>[\s\S]*?<\/div>/; - - if (!rootContainerPattern.test(template)) { + const openTagMatch = template.match(/<div\s+id=(?:"root"|'root'|root)[^>]*>/); + + if (!openTagMatch) { throw new Error('[ssr-toolkit] failed to locate root container in built html template.'); } - - return template.replace(rootContainerPattern, `<div id="root">${appHtml}</div>`); + + const start = openTagMatch.index; + const afterOpen = start + openTagMatch[0].length; + let depth = 1; + let pos = afterOpen; + + while (pos < template.length && depth > 0) { + const nextOpen = template.indexOf('<div', pos); + const nextClose = template.indexOf('</div>', pos); + if (nextClose === -1) break; + if (nextOpen !== -1 && nextOpen < nextClose) { + depth++; + pos = nextOpen + 4; + } else { + depth--; + if (depth === 0) { + return ( + template.slice(0, start) + + `<div id="root">${appHtml}</div>` + + template.slice(nextClose + 6) + ); + } + pos = nextClose + 6; + } + } + + throw new Error('[ssr-toolkit] failed to locate root container in built html template.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/server/template.mjs` around lines 26 - 32, The current rootContainerPattern regex (rootContainerPattern) uses a lazy quantifier and can stop at the first nested </div>, producing dangling tags; instead parse the template into a DOM and locate the element with id "root" (e.g., create a DOM parser/JSDOM or use a lightweight HTML parser), replace that element's innerHTML with appHtml, and serialize back to string rather than using template.replace with rootContainerPattern; update the code around rootContainerPattern, the test that throws the Error, and the return that calls template.replace so they operate on the parsed DOM (find element by id "root", ensure it exists, set its innerHTML to appHtml, then return the serialized document).packages/ssr/src/index.ts-35-53 (1)
35-53:⚠️ Potential issue | 🟡 MinorExport
SsrUpdateItemandSsrUpdateResultfrom the public API.
SsrDevCompilerLike(re-exported from./dev-server.js) declaresupdate(paths: SsrUpdateItem[]): Promise<SsrUpdateResult>. Implementers providing a custom server compiler must import bothSsrUpdateItemandSsrUpdateResultto properly type this method, but neither type is currently exported from the public API surface. Add both to the re-export list inpackages/ssr/src/index.ts.♻️ Proposed addition
export type { SsrDevCompilerLike, SsrDevFarmServerLike, SsrDevHostServerLike, SsrDevServer, SsrDevServerCompilerCreateResult, SsrDevServerFactories, SsrDevServerListenOptions, SsrDevServerOptions, SsrDevWatcherLike, SsrMiddleware, SsrMiddlewareServer, SsrNextMiddleware, SsrRenderContext, SsrRenderOptions, + SsrUpdateItem, + SsrUpdateResult, SsrTemplateLoadContext, SsrTemplateOptions, SsrTemplateRenderContext } from './dev-server.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/index.ts` around lines 35 - 53, The public API is missing SsrUpdateItem and SsrUpdateResult which are required by the SsrDevCompilerLike.update signature; update the re-export block in packages/ssr/src/index.ts to also export SsrUpdateItem and SsrUpdateResult from './dev-server.js' so implementers can import those types used by SsrDevCompilerLike.update(paths: SsrUpdateItem[]): Promise<SsrUpdateResult>.
| hmr: { | ||
| port: 9801 | ||
| }, | ||
| middlewares |
There was a problem hiding this comment.
middlewares is an undefined identifier — compile error / runtime ReferenceError.
The shorthand property syntax { middlewares } expands to { middlewares: middlewares }, but middlewares is never imported or declared in this file. TypeScript will reject it with Cannot find name 'middlewares', and at runtime it will throw a ReferenceError. Either import/declare the middleware array, or remove the line if middleware wiring lives elsewhere (e.g., in server.mjs).
🐛 Example fix
import { defineConfig } from '@farmfe/core';
+import type { UserConfig } from '@farmfe/core';
+
+// Define or import your middleware array
+const middlewares: UserConfig['server']['middlewares'] = [
+ // ... your middleware entries
+];
export default defineConfig({
server: {
middlewareMode: true,
hmr: {
port: 9801
- },
- middlewares
+ }
+ // middlewares wired via server.mjs, not here
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/middleware-mode/farm.config.ts` at line 9, The file references an
undefined identifier middlewares via shorthand object property (the `{
middlewares }` line) causing a compile/runtime error; either import or declare
the middleware array (e.g., add an import or const middlewares = [...] in this
module) and use that symbol, or remove the middlewares property from the
exported config if middleware wiring is handled elsewhere (e.g., in server.mjs)
— update the `middlewares` reference in farm.config.ts accordingly.
middlewares is undefined — compile error / runtime ReferenceError.
The shorthand property middlewares on line 9 expands to middlewares: middlewares, but middlewares is never imported or declared in this file. TypeScript will refuse to compile it (Cannot find name 'middlewares'), and the runtime will throw a ReferenceError.
Either import/define the middleware array before the config object, or remove the line if middleware is handled elsewhere:
🐛 Example fix (remove the dangling reference)
import { defineConfig } from '@farmfe/core';
+import { middlewares } from './middlewares.js'; // adjust import to actual source
export default defineConfig({
server: {
middlewareMode: true,
hmr: {
port: 9801
- },
- middlewares
+ }
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/middleware-mode/farm.config.ts` at line 9, The config object
contains a dangling shorthand property "middlewares" that references an
undeclared identifier and causes a compile/runtime error; either remove the
"middlewares" property from the exported config or import/declare the middleware
array before the config (e.g., define const middlewares: Middleware[] = [...] or
import { middlewares } from '...') and ensure the exported config uses that
declared symbol (the shorthand "middlewares" within the config object or an
explicit "middlewares: middlewares" assignment).
| ```bash | ||
| scripts/pnpm-node22.sh --filter @farmfe/cli build | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the referenced wrapper script and server module files exist.
echo "=== Checking scripts/pnpm-node22.sh ==="
fd -t f "pnpm-node22.sh" scripts/
echo ""
echo "=== Checking if file is executable ==="
if [ -f "scripts/pnpm-node22.sh" ]; then
ls -la scripts/pnpm-node22.sh
else
echo "File not found"
fi
echo ""
echo "=== Checking server/ module files referenced in README ==="
fd -t f -e mjs . examples/ssr-toolkit/server/Repository: farm-fe/farm
Length of output: 380
The script scripts/pnpm-node22.sh does not exist in the repository. The bash command shown in the README references a non-existent file, which will cause the example to fail when users attempt to follow it. Either create the missing script or update the command to use the correct path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/ssr-toolkit/README.md` around lines 22 - 24, The README references a
non-existent script scripts/pnpm-node22.sh in the example command; update the
README to point to the correct existing script or add the missing script. Locate
the example command line containing "scripts/pnpm-node22.sh --filter `@farmfe/cli`
build" and either replace scripts/pnpm-node22.sh with the proper script
path/name used in the repo (or a generic alternative like pnpm) or create the
scripts/pnpm-node22.sh wrapper script that sets Node 22 and runs the given pnpm
command so the example works as written.
| fetchModule( | ||
| id: string, | ||
| importer?: string, | ||
| options?: JsFetchModuleOptions | ||
| ): JsFetchModuleResult | null { | ||
| const bindingCompiler = this._bindingCompiler as BindingCompiler & { | ||
| fetchModule?: ( | ||
| id: string, | ||
| importer?: string | null, | ||
| options?: JsFetchModuleOptions | null | ||
| ) => JsFetchModuleResult | null; | ||
| }; | ||
|
|
||
| if (typeof bindingCompiler.fetchModule !== 'function') { | ||
| return null; | ||
| } | ||
|
|
||
| return (bindingCompiler.fetchModule( | ||
| id, | ||
| importer ?? null, | ||
| options ?? null | ||
| ) ?? null) as JsFetchModuleResult | null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the binding definition files
fd -type f -name "binding.d.ts" -o -name "binding.js" | head -20Repository: farm-fe/farm
Length of output: 226
🏁 Script executed:
# Look for the binding.d.ts file structure
find . -name "binding.d.ts" -type fRepository: farm-fe/farm
Length of output: 92
🏁 Script executed:
# Examine the types in src/types/binding.ts and binding.d.ts
fd -type f "binding.ts" "binding.d.ts" | xargs wc -lRepository: farm-fe/farm
Length of output: 245
🏁 Script executed:
# Read the binding.d.ts file to check JsFetchModuleResult definition
cat ./packages/core/binding/binding.d.tsRepository: farm-fe/farm
Length of output: 3796
🏁 Script executed:
# Find and read src/types/binding.ts
find . -path "*/src/types/binding.ts" -type f | head -1 | xargs catRepository: farm-fe/farm
Length of output: 18152
🏁 Script executed:
# Search for fetchModule usage (corrected rg command without -type flag)
rg -n 'fetchModule' --glob '!binding.d.ts' --glob '!node_modules' -A 2 -B 2Repository: farm-fe/farm
Length of output: 50368
🏁 Script executed:
# Find the isFetchResult function definition
rg -n "isFetchResult" packages/core/src/module-runner/serverInvoke.ts -A 10 -B 2Repository: farm-fe/farm
Length of output: 795
🏁 Script executed:
# Look at the validation logic in serverInvoke.ts around lines 484-488
sed -n '480,495p' packages/core/src/module-runner/serverInvoke.tsRepository: farm-fe/farm
Length of output: 455
🏁 Script executed:
# Check if there's any type validation or normalization happening
rg -n "function isFetchResult|const isFetchResult" packages/core/src -A 15Repository: farm-fe/farm
Length of output: 1265
🏁 Script executed:
# Get the complete isFetchResult function
sed -n '173,220p' packages/core/src/module-runner/serverInvoke.tsRepository: farm-fe/farm
Length of output: 1236
🏁 Script executed:
# Also check what FetchResult type is
rg -n "type FetchResult|interface FetchResult" packages/core/src/module-runner/serverInvoke.ts -A 20Repository: farm-fe/farm
Length of output: 38
🏁 Script executed:
# Find FetchResult type definition
rg -n "type FetchResult" packages/core/src/module-runner/ -B 2 -A 10Repository: farm-fe/farm
Length of output: 877
🏁 Script executed:
# Also look at the imports in serverInvoke.ts
head -50 packages/core/src/module-runner/serverInvoke.ts | grep -A 5 -B 5 "import"Repository: farm-fe/farm
Length of output: 504
🏁 Script executed:
# Let me check the exact types in module-runner/types.ts to compare with src/types/binding.ts
sed -n '60,92p' packages/core/src/module-runner/types.tsRepository: farm-fe/farm
Length of output: 837
🏁 Script executed:
# Check if JsFetchModuleResult is actually used or if it's converted to FetchResult
rg -n "JsFetchModuleResult" packages/core/src/module-runner/ -B 2 -A 2Repository: farm-fe/farm
Length of output: 38
🏁 Script executed:
# Search across the entire codebase for JsFetchModuleResult
rg -n "JsFetchModuleResult" packages/core/src -B 2 -A 2Repository: farm-fe/farm
Length of output: 1665
🏁 Script executed:
# Look at the compiler/index.ts imports to see what's being imported
head -30 packages/core/src/compiler/index.tsRepository: farm-fe/farm
Length of output: 939
🏁 Script executed:
# Check the comment at the top of binding.d.ts
head -5 packages/core/binding/binding.d.tsRepository: farm-fe/farm
Length of output: 200
🏁 Script executed:
# Look at how the Rust binding actually implements fetchModule
fd -type f "*.rs" packages/core | xargs rg -l "fetchModule" | head -5Repository: farm-fe/farm
Length of output: 447
🏁 Script executed:
# Check how the actual binding result is constructed - look at test mocks vs actual usage
sed -n '3282,3320p' packages/core/tests/module-runner.spec.tsRepository: farm-fe/farm
Length of output: 1257
🏁 Script executed:
# Let me check what happens when fetchModule is called - trace the actual return
rg -n "compiler.fetchModule" packages/core/src/module-runner/serverInvoke.ts -B 5 -A 10Repository: farm-fe/farm
Length of output: 470
🏁 Script executed:
# Check if there's any transformation/normalization of the result
sed -n '411,445p' packages/core/src/module-runner/serverInvoke.tsRepository: farm-fe/farm
Length of output: 892
🏁 Script executed:
# Look at normalizeInlinedFetchResult and how results are actually used after validation
sed -n '216,260p' packages/core/src/module-runner/serverInvoke.tsRepository: farm-fe/farm
Length of output: 1142
🏁 Script executed:
# Check the full resolveFetchModule function to see what happens after isFetchResult validation
sed -n '438,510p' packages/core/src/module-runner/serverInvoke.tsRepository: farm-fe/farm
Length of output: 1887
🏁 Script executed:
# Let me verify the return type signature in binding.d.ts for fetchModule method
sed -n '20,25p' packages/core/binding/binding.d.tsRepository: farm-fe/farm
Length of output: 378
The type mismatch is real but partially mitigated by runtime validation.
The Rust binding declares JsFetchModuleResult as a flat interface with all optional fields (binding.d.ts lines 52-63), but src/types/binding.ts shadows this with a discriminated union requiring specific fields per variant (lines 44-47: JsCachedFetchResult | JsExternalizedFetchResult | JsInlinedFetchResult). The cast on line 197 silently bridges these incompatible shapes.
However, the impact is partially mitigated: serverInvoke.ts validates results against the union structure via isFetchResult() (lines 173-213), which would reject malformed shapes. This guards against the type mismatch at runtime but doesn't resolve the underlying type unsoundness.
The unsafe cast remains problematic because:
- It bypasses TypeScript type safety for this return path
- Future refactoring could break if validation is removed or changed
- It creates unnecessary complexity in the type system
Either align the TS union variants to match the binding's flat-optional shape, or make the binding's type declaration explicit about the discriminated union it actually returns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/compiler/index.ts` around lines 176 - 198, The fetchModule
method is unsafely casting the Rust binding's flat-optional JsFetchModuleResult
into a stricter discriminated union (defined in src/types/binding.ts), bypassing
TypeScript checks; instead, update types so they match: either change the
binding declaration of JsFetchModuleResult to express the discriminated union
shape the runtime actually returns, or relax the TS union to accept the
flat-optional form returned by BindingCompiler.fetchModule, then remove the
unsafe cast in fetchModule and rely on the existing runtime guard isFetchResult
(from serverInvoke.ts) to validate results; locate and adjust the types around
BindingCompiler, fetchModule, and JsFetchModuleResult so the compile-time types
align with runtime validation.
| export class BunModulesEvaluator extends BaseAsyncFunctionEvaluator { | ||
| constructor(resolveExternalModule?: ExternalModuleResolver) { | ||
| super('bun', false, resolveExternalModule); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bun CommonJS require support in ESM contexts native
💡 Result:
Bun’s runtime has native CommonJS + ESM interop, and (unlike Node.js ESM) it lets you call require() from an ES module.
-
In Bun,
requirecan be used by both ES modules and CommonJS modules. When yourequire():- an ES module, Bun returns the module namespace object (similar to
import * as). - a CommonJS module, Bun returns
module.exports.
[1]
- an ES module, Bun returns the module namespace object (similar to
-
Bun also explicitly documents that you can
require()(orimport) CommonJS from ESM as a supported pattern. [3]
If you’re targeting Node.js compatibility, note this is a Bun runtime feature; Node’s ESM generally doesn’t provide a global require() the same way. [1]
🏁 Script executed:
cd /tmp && cat > find_bun_evaluator.sh << 'EOF'
#!/bin/bash
# Find the BunModulesEvaluator class and its constructor parameters
rg -A 5 "class BunModulesEvaluator" packages/core/src/module-runner/evaluator.ts
EOF
bash find_bun_evaluator.shRepository: farm-fe/farm
Length of output: 140
🏁 Script executed:
# Check the BaseAsyncFunctionEvaluator constructor to see what allowCommonJsRequire parameter does
rg -B 5 -A 10 "constructor.*allowCommonJsRequire" packages/core/src/module-runner/evaluator.tsRepository: farm-fe/farm
Length of output: 38
🏁 Script executed:
# Find where nodeCommonJsLoader is used to understand the implications
rg -B 2 -A 5 "nodeCommonJsLoader" packages/core/src/module-runner/evaluator.tsRepository: farm-fe/farm
Length of output: 1249
🏁 Script executed:
find . -name "evaluator.ts" -type f 2>/dev/nullRepository: farm-fe/farm
Length of output: 102
🏁 Script executed:
# List the module-runner directory structure
find . -path "*/module-runner*" -type d 2>/dev/null | head -20Repository: farm-fe/farm
Length of output: 89
🏁 Script executed:
# Get the BunModulesEvaluator class definition
sed -n '500,522p' ./packages/core/src/module-runner/evaluator.tsRepository: farm-fe/farm
Length of output: 817
🏁 Script executed:
# Get all evaluator class definitions for comparison
sed -n '500,522p' ./packages/core/src/module-runner/evaluator.ts | cat -n
# Also get ESModulesEvaluator and others
rg -A 3 "class (BunModulesEvaluator|DenoModulesEvaluator|WorkerModulesEvaluator|ESModulesEvaluator)" ./packages/core/src/module-runner/evaluator.tsRepository: farm-fe/farm
Length of output: 1738
BunModulesEvaluator should enable CommonJS support — Bun natively supports require() in ESM contexts.
BunModulesEvaluator passes allowCommonJsRequire: false, but Bun has built-in support for require() from ES modules. Unlike Node.js strict ESM, Bun explicitly allows calling require() for both CJS and ESM modules. SSR modules that depend on CommonJS-only packages will fail with "commonjs external module requires Node module loader support" instead of loading via Bun's native require, even though Bun can handle this natively. Change to super('bun', true, resolveExternalModule) to align with ESModulesEvaluator and Bun's actual capabilities.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/module-runner/evaluator.ts` around lines 512 - 516,
BunModulesEvaluator currently calls super('bun', false, resolveExternalModule)
which disables CommonJS require support; update the BunModulesEvaluator
constructor to call super('bun', true, resolveExternalModule) so Bun's native
require() in ESM contexts is enabled (modify the constructor in the
BunModulesEvaluator class that extends BaseAsyncFunctionEvaluator to pass true
for allowCommonJsRequire).
| const changedModules = [ | ||
| ...result.changed, | ||
| ...result.added, | ||
| ...result.extraWatchResult.add | ||
| ]; | ||
|
|
||
| if (changedModules.length > 0) { | ||
| const updates: RunnerHotUpdate[] = changedModules.map((item) => ({ | ||
| type: 'js-update', | ||
| path: item, | ||
| acceptedPath: item, | ||
| timestamp: Date.now() | ||
| })); | ||
|
|
||
| onMessage({ | ||
| type: 'update', | ||
| updates | ||
| } satisfies RunnerHotPayload); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file structure and the full function context
cd packages/core/src/module-runner && wc -l serverTransport.tsRepository: farm-fe/farm
Length of output: 78
🏁 Script executed:
# Get the complete function context around these lines
cd packages/core/src/module-runner && sed -n '60,110p' serverTransport.tsRepository: farm-fe/farm
Length of output: 1318
🏁 Script executed:
# Check JsUpdateResult type definition
rg -n 'type JsUpdateResult|interface JsUpdateResult' packages/core/src/types/ -A15Repository: farm-fe/farm
Length of output: 38
🏁 Script executed:
# Search for how removed is used in this context
rg -n 'result\.removed' packages/core/src/module-runner/serverTransport.ts -B5 -A5Repository: farm-fe/farm
Length of output: 341
🏁 Script executed:
# Search more broadly for JsUpdateResult definition
rg -n 'JsUpdateResult' packages/core/src/ -B2 -A10 | head -100Repository: farm-fe/farm
Length of output: 6895
🏁 Script executed:
# Search in binding.ts or types directory
find packages/core/src -name "*.ts" -type f | xargs grep -l "JsUpdateResult" | head -5Repository: farm-fe/farm
Length of output: 250
🏁 Script executed:
# Check how the hmrEngine creates JsUpdateResult
rg -n 'onUpdateFinish|JsUpdateResult' packages/core/src -B3 -A3 | head -80Repository: farm-fe/farm
Length of output: 5486
🏁 Script executed:
# Find the JsUpdateResult type definition
find packages -name "*.ts" -path "*/types/*" | xargs grep -l "JsUpdateResult"Repository: farm-fe/farm
Length of output: 40
🏁 Script executed:
# Look for binding.ts or similar
fd -e ts "binding" packages/core/src/types/Repository: farm-fe/farm
Length of output: 90
🏁 Script executed:
# Search for the type/interface definition more broadly
rg -n "type JsUpdateResult|interface JsUpdateResult|export.*JsUpdateResult" packages/core/src/types/ -A20Repository: farm-fe/farm
Length of output: 38
🏁 Script executed:
# Read the binding.ts file to find JsUpdateResult definition
cat -n packages/core/src/types/binding.ts | grep -A25 "JsUpdateResult"Repository: farm-fe/farm
Length of output: 38
🏁 Script executed:
# Get more context on JsUpdateResult
rg "JsUpdateResult" packages/core/src/types/binding.ts -A20 -B2Repository: farm-fe/farm
Length of output: 38
🏁 Script executed:
# Let's read the binding.ts file directly
cat packages/core/src/types/binding.tsRepository: farm-fe/farm
Length of output: 18152
🏁 Script executed:
# Find the binding directory and look for type definitions
find packages -type d -name "binding" | head -5Repository: farm-fe/farm
Length of output: 114
🏁 Script executed:
# Search for JsUpdateResult in all files to understand its structure
rg "removed.*:" packages/core/src/server/hmr-engine.ts -B5 -A5Repository: farm-fe/farm
Length of output: 518
🏁 Script executed:
# Look at how updateResult is used in hmr-engine and watcher
rg "result\.removed|result\.changed|result\.added" packages/core/src/ -B2 -A2 | head -80Repository: farm-fe/farm
Length of output: 869
🏁 Script executed:
# Look at how the result object is constructed in hmr-engine
sed -n '32,120p' packages/core/src/server/hmr-engine.tsRepository: farm-fe/farm
Length of output: 2822
🏁 Script executed:
# Check if there are any scenarios where both changed and removed could be non-empty
rg "callUpdates|recompileAndSendResult" packages/core/src/server/hmr-engine.ts -B3 -A15 | head -100Repository: farm-fe/farm
Length of output: 2521
When an update contains both changed/added modules and removals, removals are silently dropped.
The early return at line 90 prevents the result.removed check at line 93 from executing when changedModules is non-empty. Since the compiler can populate changed, added, and removed independently in a single update batch, removals may be lost if concurrent with any changes or additions, leaving stale code in the module cache.
Consider whether removals should be handled independently (perhaps by triggering a full reload when any removals are present), or by processing both branches regardless of the presence of changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/module-runner/serverTransport.ts` around lines 72 - 91, The
current logic builds changedModules and returns early after sending a 'update'
via onMessage, which causes result.removed to be ignored; update the
serverTransport handler so removals are not dropped—either remove the early
return and process result.removed after sending the 'update' (sending a separate
removal payload or invoking the existing removal handling), or detect any
removals and trigger a full reload (e.g., send a 'full-reload' RunnerHotPayload)
when result.removed.length > 0; ensure you reference changedModules,
result.removed, onMessage, RunnerHotUpdate and RunnerHotPayload so both
changes/additions and removals are handled in the same update batch.
| }; | ||
|
|
||
| server.hmrEngine?.onUpdateFinish(updateListener); | ||
|
|
||
| disconnectHandler = () => { | ||
| active = false; | ||
| onDisconnection(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the serverTransport.ts file
fd -type f -name "serverTransport.ts"Repository: farm-fe/farm
Length of output: 226
🏁 Script executed:
# Find the serverTransport.ts file
fd "serverTransport.ts"Repository: farm-fe/farm
Length of output: 106
🏁 Script executed:
# Read the serverTransport.ts file to examine lines 96-103 and surrounding context
cat -n packages/core/src/module-runner/serverTransport.tsRepository: farm-fe/farm
Length of output: 3602
🏁 Script executed:
# Find hmrEngine definition - search for the HmrEngine interface or class
fd -name "*.ts" | xargs rg "interface.*HmrEngine|class.*HmrEngine" -A 5Repository: farm-fe/farm
Length of output: 730
🏁 Script executed:
# Find the onUpdateFinish method definition in hmr-engine.ts
rg "onUpdateFinish" packages/core/src/server/hmr-engine.ts -A 10Repository: farm-fe/farm
Length of output: 720
🏁 Script executed:
# Check how _onUpdates array is used and if there's any removal mechanism
rg "_onUpdates" packages/core/src/server/hmr-engine.tsRepository: farm-fe/farm
Length of output: 251
🏁 Script executed:
# Read the full HmrEngine class to understand the listener registration and usage
sed -n '1,200p' packages/core/src/server/hmr-engine.ts | cat -nRepository: farm-fe/farm
Length of output: 6894
🏁 Script executed:
# Check if there's any method to remove listeners from HmrEngine
rg "removeListener|unsubscribe|_onUpdates.splice|_onUpdates.filter" packages/core/src/server/hmr-engine.tsRepository: farm-fe/farm
Length of output: 38
Listener leak: updateListener is never removed from hmrEngine on disconnect.
In createModuleRunnerTransportFromInvokeHandlers (line 40), unsubscribe() properly removes the hot-bus listener. Here, the updateListener registered via server.hmrEngine?.onUpdateFinish() has no removal mechanism — active = false only prevents message forwarding, but the callback remains in the HmrEngine's _onUpdates array indefinitely. On repeated connect/disconnect cycles, listeners accumulate and are invoked on every HMR update, causing a memory leak.
The proposed fix in the original comment is incorrect: HmrEngine.onUpdateFinish() returns void, not an unsubscribe function. A proper fix requires modifying HmrEngine to support listener removal (e.g., return an unsubscribe function from onUpdateFinish, add a removeListener method, or similar), then use that mechanism in the disconnectHandler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/module-runner/serverTransport.ts` around lines 96 - 103,
The registered updateListener passed to server.hmrEngine?.onUpdateFinish(...) is
never removed on disconnect which leaks listeners; update HmrEngine to support
unsubscribing (e.g., change onUpdateFinish to return an unsubscribe function or
add a removeOnUpdateFinish/removeListener API) and then capture that unsubscribe
when calling server.hmrEngine?.onUpdateFinish(...) inside
createModuleRunnerTransportFromInvokeHandlers; call the unsubscribe (or
appropriate remove method) from disconnectHandler instead of only setting active
= false so the listener is actually removed on disconnect (also mirror the
pattern used by unsubscribe() for the hot-bus listener).
| } else { | ||
| result.push(...mods.map((mod) => mod.id)); | ||
| for (const mod of mods) { | ||
| pushResult(mod.id, updateType); | ||
| } | ||
| } |
There was a problem hiding this comment.
mods is iterated without a null guard — runtime crash when getModulesByFile returns undefined.
Vite's getModulesByFile signature is (file: string): Set<ModuleNode> | undefined. The as unknown as ModuleNode[] cast on line 695 hides this. The HmrContext construction on line 703 is already null-safe (mods ?? []), but the fallback loop below is not:
} else {
for (const mod of mods) { // 💥 TypeError when mods is undefinedIf a file update arrives for a path not in the module graph and the plugin's handleHotUpdate returns undefined (or the plugin has no hook), this path crashes.
🛡️ Proposed fix
- for (const mod of mods) {
+ for (const mod of (mods ?? [])) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/plugin/js/vite-plugin-adapter.ts` around lines 723 - 727,
The code iterates over mods without guarding for undefined, causing a crash when
getModulesByFile returns undefined; update the fallback branch so it uses a
null-safe iteration (e.g., iterate over mods ?? [] or check if mods is truthy)
before calling pushResult; modify the block containing the for (const mod of
mods) loop in vite-plugin-adapter.ts to use the same null-safe pattern already
used when constructing HmrContext (referencing getModulesByFile, mods, and
pushResult) so it no longer throws when mods is undefined.
| } else { | ||
| const placeholder = templateOptions.placeholder ?? '<!--app-html-->'; | ||
|
|
||
| if (!template.includes(placeholder)) { | ||
| throw new Error( | ||
| `[farm ssr] template placeholder "${placeholder}" was not found.` | ||
| ); | ||
| } | ||
|
|
||
| html = template.replace(placeholder, appHtml); | ||
| } |
There was a problem hiding this comment.
String.replace interprets $ replacement patterns in appHtml.
If the SSR-rendered appHtml contains dollar-sign sequences ($&, $`, $', $1–$9), template.replace(placeholder, appHtml) will interpret them as special replacement patterns, silently corrupting the output. Use a function replacer to avoid this.
Proposed fix
- html = template.replace(placeholder, appHtml);
+ html = template.replace(placeholder, () => appHtml);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| const placeholder = templateOptions.placeholder ?? '<!--app-html-->'; | |
| if (!template.includes(placeholder)) { | |
| throw new Error( | |
| `[farm ssr] template placeholder "${placeholder}" was not found.` | |
| ); | |
| } | |
| html = template.replace(placeholder, appHtml); | |
| } | |
| } else { | |
| const placeholder = templateOptions.placeholder ?? '<!--app-html-->'; | |
| if (!template.includes(placeholder)) { | |
| throw new Error( | |
| `[farm ssr] template placeholder "${placeholder}" was not found.` | |
| ); | |
| } | |
| html = template.replace(placeholder, () => appHtml); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/build-preview.ts` around lines 491 - 501, The template
replacement uses template.replace(placeholder, appHtml) which allows $-sequences
in appHtml to be treated as special replacement patterns; update the replacement
in build-preview.ts to use a function replacer so the raw appHtml is inserted
verbatim (e.g., call template.replace(placeholder, () => appHtml)); reference
templateOptions.placeholder, placeholder, template, appHtml and html when making
the change so the placeholder check remains and html is set from the
function-based replacement to avoid $-pattern interpretation.
| const placeholder = params.options.template.placeholder ?? '<!--app-html-->'; | ||
| if (!template.includes(placeholder)) { | ||
| throw new Error( | ||
| `[farm ssr] template placeholder "${placeholder}" was not found.` | ||
| ); | ||
| } | ||
|
|
||
| return template.replace(placeholder, appHtml); |
There was a problem hiding this comment.
Same String.replace $-pattern issue as in build-preview.ts.
template.replace(placeholder, appHtml) on line 703 will misinterpret dollar-sign sequences in appHtml as replacement patterns. Use a function replacer.
Proposed fix
- return template.replace(placeholder, appHtml);
+ return template.replace(placeholder, () => appHtml);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const placeholder = params.options.template.placeholder ?? '<!--app-html-->'; | |
| if (!template.includes(placeholder)) { | |
| throw new Error( | |
| `[farm ssr] template placeholder "${placeholder}" was not found.` | |
| ); | |
| } | |
| return template.replace(placeholder, appHtml); | |
| const placeholder = params.options.template.placeholder ?? '<!--app-html-->'; | |
| if (!template.includes(placeholder)) { | |
| throw new Error( | |
| `[farm ssr] template placeholder "${placeholder}" was not found.` | |
| ); | |
| } | |
| return template.replace(placeholder, () => appHtml); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/dev-server.ts` around lines 696 - 703, The template
replacement uses template.replace(placeholder, appHtml) which treats $ sequences
in appHtml as replacement patterns; update the replacement to use a function
replacer so appHtml is inserted verbatim (e.g., change the call in the block
that defines placeholder and returns template.replace(...) to use a function
that returns appHtml), ensuring any $ in appHtml are not interpreted; keep the
same placeholder constant and return value.
| interface SsrServerResolvers { | ||
| createDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>; | ||
| createPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>; | ||
| startDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>; | ||
| startPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
SsrServerResolvers must be exported — it's a parameter type of two exported functions.
createSsrServerWithResolvers and startSsrServerWithResolvers are public API, but consumers cannot declare a typed variable for their custom resolvers because the interface isn't accessible. They are forced to use the awkward Parameters<typeof createSsrServerWithResolvers>[1] workaround.
♻️ Proposed fix
-interface SsrServerResolvers {
+export interface SsrServerResolvers {
createDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>;
createPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>;
startDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>;
startPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface SsrServerResolvers { | |
| createDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>; | |
| createPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>; | |
| startDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>; | |
| startPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>; | |
| } | |
| export interface SsrServerResolvers { | |
| createDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>; | |
| createPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>; | |
| startDevServer(options: SsrDevServerOptions): Promise<SsrDevServer>; | |
| startPreviewServer(options: SsrPreviewOptions): Promise<SsrPreviewServer>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/server.ts` around lines 32 - 37, The interface
SsrServerResolvers is currently unexported but is used as the parameter type for
the public functions createSsrServerWithResolvers and
startSsrServerWithResolvers; export the interface (export interface
SsrServerResolvers) so consumers can import and declare typed custom resolvers
instead of using Parameters<typeof createSsrServerWithResolvers>[1]; update any
related re-exports if needed so SsrServerResolvers is part of the package's
public API surface.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
packages/ssr/src/build-preview.ts (1)
532-540:⚠️ Potential issue | 🟠 MajorUse a function replacer for
appHtml.Line 540 passes SSR output as a string replacement, so
$&,$1, and similar sequences insideappHtmlare interpreted byString.replaceand can corrupt the response body. Switch this totemplate.replace(placeholder, () => appHtml).Proposed fix
- html = template.replace(placeholder, appHtml); + html = template.replace(placeholder, () => appHtml);What replacement patterns does JavaScript `String.prototype.replace` interpret when the replacement value is a string?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/build-preview.ts` around lines 532 - 540, The current code uses template.replace(placeholder, appHtml) which treats appHtml as a string replacement so sequences like $&, $1, etc. inside appHtml can be interpreted and corrupt the output; change the call to use a function replacer instead (i.e., call template.replace(placeholder, () => appHtml)) so appHtml is inserted verbatim. Update the replacement where template, placeholder (from templateOptions.placeholder) and appHtml are used and ensure template.replace is the only change.packages/ssr/src/dev-server.ts (1)
590-614:⚠️ Potential issue | 🟠 MajorInject
appHtmlverbatim and preserve the matched root container.Both replacement branches pass
appHtmltoString.replaceas a plain string, so$sequences in SSR output are interpreted as replacement patterns. The fallback branch also recreates<div id="root">from scratch and drops any existing attributes on the matched root node. Use function replacers and reuse the original opening/closing tags.Proposed fix
-const ROOT_CONTAINER_PATTERN = - /<div\s+id=(?:"root"|'root'|root)[^>]*>[\s\S]*?<\/div>/; +const ROOT_CONTAINER_PATTERN = + /(<div\s+id=(?:"root"|'root'|root)[^>]*>)[\s\S]*?(<\/div>)/; @@ if (params.template.includes(params.placeholder)) { - return params.template.replace(params.placeholder, params.appHtml); + return params.template.replace(params.placeholder, () => params.appHtml); } @@ return params.template.replace( ROOT_CONTAINER_PATTERN, - `<div id="root">${params.appHtml}</div>` + (_match, openTag: string, closeTag: string) => + `${openTag}${params.appHtml}${closeTag}` ); }What replacement patterns does JavaScript `String.prototype.replace` interpret when the replacement value is a string?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/dev-server.ts` around lines 590 - 614, The injectAppHtmlToTemplate function currently passes appHtml as a plain string to String.replace, which treats $ sequences as replacement patterns and rebuilds the root node losing attributes; change both replace calls to use a function replacer so appHtml is inserted verbatim (use () => params.appHtml for the placeholder branch) and change the ROOT_CONTAINER_PATTERN replace to a function replacer that reuses the matched opening/closing tag groups from the regex match (return opening + params.appHtml + closing) instead of reconstructing a <div id="root">, ensuring the original attributes and tags from the regex match are preserved and no $-escape interpretation occurs.
🧹 Nitpick comments (2)
packages/core/src/module-runner/runner.ts (2)
268-281: Consider handling HMR connection errors.The HMR connection is initiated with
voidto ignore the promise, but iftransport.connectthrows or fails, the error will be silently swallowed. Consider adding error handling for robustness during development.♻️ Suggested improvement
if (options.hmr !== false) { if (!this.options.transport.connect) { throw new Error( '[farm module runner] HMR is enabled but transport.connect is not available.' ); } - void this.options.transport.connect({ + this.options.transport.connect({ onMessage: (payload) => this.handleHotPayload(payload), onDisconnection: () => { // No-op for now. Keep room for future reconnect strategy. } + }).catch((err) => { + console.warn('[farm module runner] HMR connection failed:', err); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/module-runner/runner.ts` around lines 268 - 281, The HMR connect promise is ignored (void this.options.transport.connect(...)) so connection failures are swallowed; update the HMR block (the options.hmr check and this.options.transport.connect call) to handle rejections by awaiting or attaching .catch and logging/propagating errors and optionally retrying; ensure the connect invocation still passes the same callbacks (onMessage -> this.handleHotPayload, onDisconnection) but add a .catch handler or try/catch around the await to call the appropriate logger/error handler and avoid silent failures.
614-622:import.meta.resolvethrows on async resolver — document or handle gracefully.Line 1293-1296 shows that
resolveImportMetaRequestSyncthrows if the resolver returns a promise. This is correct per spec (import.meta.resolve is sync in most runtimes), but the error message could be clearer about the expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/module-runner/runner.ts` around lines 614 - 622, The code currently sets importMeta.resolve to call resolveImportMetaRequestSync which throws if a custom resolver returns a Promise; update resolveImportMetaRequestSync to throw a clearer, actionable error message stating that import.meta.resolve must be synchronous and that the provided resolver returned a Promise (include the resolver result/type), and also update any related docs/comments near createImportMeta and importMeta.resolve to state that options.createImportMeta/resolver must be synchronous; ensure the thrown error references resolveImportMetaRequestSync and importMeta.resolve so callers can locate the issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/node/src/lib.rs`:
- Around line 973-986: The code currently calls clean_url(&raw_id).to_string()
and then returns external_fetch_result(clean_id, ...) which strips
query/fragment; instead, keep using clean_url(&raw_id) only for lookup but
preserve and reattach the original specifier's query/fragment when calling
external_fetch_result or building the returned file URL: extract the
query/fragment from raw_id (or retain raw_id) and append it to the generated URL
for data:, node:, http/https and other branches inside the externalize logic
(functions/variables: clean_url, raw_id, external_fetch_result, externalize) so
the returned externalized specifier retains the original query/fragment while
lookups still use the cleaned form.
- Around line 146-158: The hand-built path_to_file_url function must be replaced
to produce a proper percent-encoded file URL: use the url crate's
Url::from_file_path(path) (or equivalent std/OS-safe API) to convert the Path
into a file:// URL and return Url::to_string(), and handle the Result/boolean
failure path (e.g., return an error or panic with a clear message) instead of
manual string replacement; update the path_to_file_url implementation to call
Url::from_file_path(path) and return the encoded Url string so spaces, #, %, and
non-ASCII characters are correctly percent-encoded.
- Around line 966-971: The bridge currently accepts JsFetchModuleOptions in
fetch_module but ignores its fields (cached and start_offset); either implement
their behavior or remove the option from the N-API boundary. Fix by updating pub
fn fetch_module(&self, id: String, importer: Option<String>, _options:
Option<JsFetchModuleOptions>) -> Option<JsFetchModuleResult> to read the Option,
extract cached and start_offset from JsFetchModuleOptions, and pass them into
the internal fetch logic (or into whatever helper like
fetch_module_internal/fetch_from_cache is used) so cached toggles returning a
cached result and start_offset is applied when constructing the module result;
alternatively, remove the JsFetchModuleOptions parameter and all related
bindings so callers cannot pass unused options. Ensure you reference the
JsFetchModuleOptions fields (cached, start_offset) and the fetch_module function
when making the change.
- Around line 196-203: The current code uses importer_path.is_file() to decide
the base_dir which misclassifies non-existent module ids; change the logic to
treat the importer path as a directory only when importer_path.is_dir() (or it
explicitly indicates a directory), otherwise use
importer_path.parent().unwrap_or(importer_path.as_path()) as the base directory;
update the branch in the block that computes base_dir (and the analogous logic
in resolve_base_dir) to use is_dir() instead of is_file() and default to
parent() so relative imports resolve correctly for module ids that don't exist
on disk.
In `@packages/core/src/module-runner/evaluatedModules.ts`:
- Around line 74-90: The removeModule function removes the module from
idToModuleMap, urlToModuleMap and fileToModulesMap but doesn’t detach graph
edges, leaving stale references in neighbors' imports/importers; before deleting
entries in removeModule(EvaluatedModuleNode mod) call the existing
invalidateModule(mod) (or explicitly iterate mod.imports and mod.importers to
remove this mod from each neighbor’s sets) to remove all inbound/outbound edges
so the graph stays consistent.
In `@packages/core/src/module-runner/sourceMapInterceptor.ts`:
- Around line 131-142: The current global iteration over globalHookStores in
lookupSourceMap()/applyCustomStackFormatter() lets hooks from other runners
(retrieveSourceMap/formatStack) intercept stacks; restrict iterations to the
hook store(s) owned by the current runner instead of walking all entries. Change
the loops that use globalHookStores.entries() (and the calls to
resolveHookSourceCandidate, sourceCandidates and resolveHookSourceMap) to filter
by the runner identity (e.g., entry.owner or compare entry.runnerId to the
current runner context) or accept a runnerId parameter and only examine the
matching store(s) so only hooks registered by that runner are consulted; apply
the same scoping change to the other occurrences you flagged (around the blocks
at 269-288 and 476-479).
In `@packages/ssr/src/build-preview.ts`:
- Around line 117-149: The regex-only approach in rewriteModuleImportsForDataUrl
leaves relative specifiers in export ... from and dynamic import(...) forms,
which will break when code is loaded from a data: URL; update
rewriteModuleImportsForDataUrl so all relative ESM specifiers are rewritten to
absolute file:// URLs: either (A) parse the module (e.g., with a lightweight
parser or acorn/estree) and replace specifiers for ImportDeclaration,
ExportNamedDeclaration/ExportAllDeclaration and ImportExpression nodes, using
pathToFileURL(path.resolve(moduleDir, specifier)).href and keep the
createRequire replacement using originalFileUrl, or (B) avoid data: URLs
entirely by writing the transformed code to a temporary file and use its file://
URL as the module entry instead of the data: URL; adjust the code paths that
call rewriteModuleImportsForDataUrl (and the createRequire(import.meta.url)
replacement) accordingly.
In `@packages/ssr/src/dev-server.ts`:
- Around line 505-549: The watcher handlers in
watchServerCompilerWithSingleWatcher (the params.watcher.on
'add'/'unlink'/'change' callbacks and runUpdate calling params.compiler.update
and params.onUpdateResult) can run concurrently and overwrite shared
invokeContext; serialize/coalesce these updates by introducing a single async
FIFO processor: queue incoming events (path+type) from the watcher callbacks,
have a single loop/task that dequeues events and runs params.compiler.update
sequentially, and coalesce bursty events (e.g., collapse multiple events for the
same path to the latest type) before calling params.onUpdateResult so only one
update runs at a time and only the latest state is applied to the shared
invokeContext. Ensure runUpdate is adapted to enqueue rather than directly
invoke params.compiler.update and that the processor respects the active flag
and drains/cleans the queue on shutdown.
- Around line 463-489: The emitRunnerPayload function currently returns an
'update' when changed/added exist and skips 'full-reload' even if result.removed
is non-empty; change the control flow in emitRunnerPayload so that if
result.removed.length > 0 you emit a { type: 'full-reload' } (and return) before
emitting any 'update' payloads, or otherwise ensure the presence of removals
forces a full reload; update logic around the changedModules calculation and the
early return to check result.removed first (refer to emitRunnerPayload,
changedModules, and result.removed).
---
Duplicate comments:
In `@packages/ssr/src/build-preview.ts`:
- Around line 532-540: The current code uses template.replace(placeholder,
appHtml) which treats appHtml as a string replacement so sequences like $&, $1,
etc. inside appHtml can be interpreted and corrupt the output; change the call
to use a function replacer instead (i.e., call template.replace(placeholder, ()
=> appHtml)) so appHtml is inserted verbatim. Update the replacement where
template, placeholder (from templateOptions.placeholder) and appHtml are used
and ensure template.replace is the only change.
In `@packages/ssr/src/dev-server.ts`:
- Around line 590-614: The injectAppHtmlToTemplate function currently passes
appHtml as a plain string to String.replace, which treats $ sequences as
replacement patterns and rebuilds the root node losing attributes; change both
replace calls to use a function replacer so appHtml is inserted verbatim (use ()
=> params.appHtml for the placeholder branch) and change the
ROOT_CONTAINER_PATTERN replace to a function replacer that reuses the matched
opening/closing tag groups from the regex match (return opening + params.appHtml
+ closing) instead of reconstructing a <div id="root">, ensuring the original
attributes and tags from the regex match are preserved and no $-escape
interpretation occurs.
---
Nitpick comments:
In `@packages/core/src/module-runner/runner.ts`:
- Around line 268-281: The HMR connect promise is ignored (void
this.options.transport.connect(...)) so connection failures are swallowed;
update the HMR block (the options.hmr check and this.options.transport.connect
call) to handle rejections by awaiting or attaching .catch and
logging/propagating errors and optionally retrying; ensure the connect
invocation still passes the same callbacks (onMessage -> this.handleHotPayload,
onDisconnection) but add a .catch handler or try/catch around the await to call
the appropriate logger/error handler and avoid silent failures.
- Around line 614-622: The code currently sets importMeta.resolve to call
resolveImportMetaRequestSync which throws if a custom resolver returns a
Promise; update resolveImportMetaRequestSync to throw a clearer, actionable
error message stating that import.meta.resolve must be synchronous and that the
provided resolver returned a Promise (include the resolver result/type), and
also update any related docs/comments near createImportMeta and
importMeta.resolve to state that options.createImportMeta/resolver must be
synchronous; ensure the thrown error references resolveImportMetaRequestSync and
importMeta.resolve so callers can locate the issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a95f751-30b8-4175-92b1-35b792398ab6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
crates/node/src/lib.rscrates/node/src/module_runner_transform.rscrates/plugin_library/src/formats/umd.rspackages/core/src/module-runner/diagnostics.tspackages/core/src/module-runner/evaluatedModules.tspackages/core/src/module-runner/index.tspackages/core/src/module-runner/runner.tspackages/core/src/module-runner/sourceMapInterceptor.tspackages/core/src/module-runner/types.tspackages/core/tests/module-runner.benchmark.spec.tspackages/core/tests/module-runner.convergence.spec.tspackages/core/tests/module-runner.diagnostics.spec.tspackages/core/tests/module-runner.exports.spec.tspackages/core/tests/module-runner.hmr.spec.tspackages/core/tests/module-runner.import-meta.spec.tspackages/core/tests/module-runner.non-js.spec.tspackages/core/tests/module-runner.resolver.spec.tspackages/core/tests/module-runner.sourcemap.spec.tspackages/ssr/src/build-preview.tspackages/ssr/src/dev-server.tspackages/ssr/test/build-preview.spec.tspackages/ssr/test/dev-server.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/tests/module-runner.benchmark.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ssr/test/dev-server.spec.ts
- packages/core/src/module-runner/index.ts
| fn path_to_file_url(path: &Path) -> String { | ||
| let normalized = path.to_string_lossy().replace('\\', "/"); | ||
|
|
||
| if cfg!(windows) { | ||
| if normalized.starts_with('/') { | ||
| format!("file://{normalized}") | ||
| } else { | ||
| format!("file:///{normalized}") | ||
| } | ||
| } else { | ||
| format!("file://{normalized}") | ||
| } | ||
| } |
There was a problem hiding this comment.
Use a real file-URL encoder here.
This hand-built file:// string does not percent-encode spaces, #, %, or non-ASCII characters. A path containing reserved characters will externalize to the wrong URL and can fail to load on the JS side.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/lib.rs` around lines 146 - 158, The hand-built
path_to_file_url function must be replaced to produce a proper percent-encoded
file URL: use the url crate's Url::from_file_path(path) (or equivalent
std/OS-safe API) to convert the Path into a file:// URL and return
Url::to_string(), and handle the Result/boolean failure path (e.g., return an
error or panic with a clear message) instead of manual string replacement;
update the path_to_file_url implementation to call Url::from_file_path(path) and
return the encoded Url string so spaces, #, %, and non-ASCII characters are
correctly percent-encoded.
| if id.starts_with("./") || id.starts_with("../") { | ||
| let importer_path = normalize_importer_path(importer, root)?; | ||
| let base_dir = if importer_path.is_file() { | ||
| importer_path.parent().unwrap_or(importer_path.as_path()) | ||
| } else { | ||
| importer_path.as_path() | ||
| }; | ||
| return Some(base_dir.join(id)); |
There was a problem hiding this comment.
Don't use is_file() to infer importer semantics.
Many importers in this flow are module ids that are valid logically but don't exist on disk yet. In that case is_file() is false, so ./dep gets resolved as if /src/entry.mjs were a directory, producing /src/entry.mjs/dep. The same heuristic is reused for resolve_base_dir, so relative fetches can break before resolver fallback helps.
Also applies to: 1031-1042
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/lib.rs` around lines 196 - 203, The current code uses
importer_path.is_file() to decide the base_dir which misclassifies non-existent
module ids; change the logic to treat the importer path as a directory only when
importer_path.is_dir() (or it explicitly indicates a directory), otherwise use
importer_path.parent().unwrap_or(importer_path.as_path()) as the base directory;
update the branch in the block that computes base_dir (and the analogous logic
in resolve_base_dir) to use is_dir() instead of is_file() and default to
parent() so relative imports resolve correctly for module ids that don't exist
on disk.
| pub fn fetch_module( | ||
| &self, | ||
| id: String, | ||
| importer: Option<String>, | ||
| _options: Option<JsFetchModuleOptions>, | ||
| ) -> Option<JsFetchModuleResult> { |
There was a problem hiding this comment.
Either honor JsFetchModuleOptions or drop it from the bridge.
cached and start_offset are accepted at the N-API boundary but never read, so the new options are currently a silent no-op for callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/lib.rs` around lines 966 - 971, The bridge currently accepts
JsFetchModuleOptions in fetch_module but ignores its fields (cached and
start_offset); either implement their behavior or remove the option from the
N-API boundary. Fix by updating pub fn fetch_module(&self, id: String, importer:
Option<String>, _options: Option<JsFetchModuleOptions>) ->
Option<JsFetchModuleResult> to read the Option, extract cached and start_offset
from JsFetchModuleOptions, and pass them into the internal fetch logic (or into
whatever helper like fetch_module_internal/fetch_from_cache is used) so cached
toggles returning a cached result and start_offset is applied when constructing
the module result; alternatively, remove the JsFetchModuleOptions parameter and
all related bindings so callers cannot pass unused options. Ensure you reference
the JsFetchModuleOptions fields (cached, start_offset) and the fetch_module
function when making the change.
| let clean_id = clean_url(&raw_id).to_string(); | ||
| let has_query = raw_id.contains('?'); | ||
|
|
||
| if clean_id.starts_with("data:") { | ||
| return Some(external_fetch_result(clean_id, "builtin".to_string(), None)); | ||
| } | ||
|
|
||
| if clean_id.starts_with("node:") { | ||
| return Some(external_fetch_result(clean_id, "builtin".to_string(), None)); | ||
| } | ||
|
|
||
| if clean_id.starts_with("http://") || clean_id.starts_with("https://") { | ||
| return Some(external_fetch_result(clean_id, "network".to_string(), None)); | ||
| } |
There was a problem hiding this comment.
Preserve query/fragment in externalize.
These paths clean or canonicalize the specifier before returning externalize, so imports like https://cdn/x.js?worker, data: URLs, or local files with ?raw/?url lose part of their identity. That changes module semantics and cache keys for the runner. Use the cleaned value only for lookup; return the original specifier or re-attach its query/fragment to the generated file URL.
Also applies to: 1018-1024, 1072-1077, 1085-1105, 1109-1114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/lib.rs` around lines 973 - 986, The code currently calls
clean_url(&raw_id).to_string() and then returns external_fetch_result(clean_id,
...) which strips query/fragment; instead, keep using clean_url(&raw_id) only
for lookup but preserve and reattach the original specifier's query/fragment
when calling external_fetch_result or building the returned file URL: extract
the query/fragment from raw_id (or retain raw_id) and append it to the generated
URL for data:, node:, http/https and other branches inside the externalize logic
(functions/variables: clean_url, raw_id, external_fetch_result, externalize) so
the returned externalized specifier retains the original query/fragment while
lookups still use the cleaned form.
| removeModule(mod: EvaluatedModuleNode): void { | ||
| this.idToModuleMap.delete(mod.id); | ||
|
|
||
| for (const [url, current] of this.urlToModuleMap.entries()) { | ||
| if (current === mod) { | ||
| this.urlToModuleMap.delete(url); | ||
| } | ||
| } | ||
|
|
||
| const fileKey = cleanUrl(mod.id); | ||
| const fileMatched = this.fileToModulesMap.get(fileKey); | ||
| if (fileMatched) { | ||
| fileMatched.delete(mod); | ||
| if (fileMatched.size === 0) { | ||
| this.fileToModulesMap.delete(fileKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
Detach graph edges before deleting the node.
removeModule() drops the maps, but it never removes this module from its neighbors' imports/importers sets. After a prune, the graph can still contain stale edges to a module that no longer exists. Calling invalidateModule(mod) first would keep the graph consistent.
Suggested fix
removeModule(mod: EvaluatedModuleNode): void {
+ this.invalidateModule(mod);
this.idToModuleMap.delete(mod.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/module-runner/evaluatedModules.ts` around lines 74 - 90,
The removeModule function removes the module from idToModuleMap, urlToModuleMap
and fileToModulesMap but doesn’t detach graph edges, leaving stale references in
neighbors' imports/importers; before deleting entries in
removeModule(EvaluatedModuleNode mod) call the existing invalidateModule(mod)
(or explicitly iterate mod.imports and mod.importers to remove this mod from
each neighbor’s sets) to remove all inbound/outbound edges so the graph stays
consistent.
| const hookEntries = [...globalHookStores.entries()].reverse(); | ||
| for (const [, entry] of hookEntries) { | ||
| for (const candidate of candidates) { | ||
| const resolved = resolveHookSourceCandidate(entry.hooks, candidate); | ||
| for (const hookCandidate of sourceCandidates(resolved)) { | ||
| const fromHook = resolveHookSourceMap(entry, hookCandidate); | ||
| if (fromHook) { | ||
| return fromHook; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Scope source-map hooks to the owning runner.
Both lookupSourceMap() and applyCustomStackFormatter() walk every registered hook store globally. If two runners are alive at once, one runner's retrieveSourceMap/formatStack hook can remap or fully replace another runner's stack trace. formatStack is especially risky here because any non-undefined return short-circuits the chain.
Also applies to: 269-288, 476-479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/module-runner/sourceMapInterceptor.ts` around lines 131 -
142, The current global iteration over globalHookStores in
lookupSourceMap()/applyCustomStackFormatter() lets hooks from other runners
(retrieveSourceMap/formatStack) intercept stacks; restrict iterations to the
hook store(s) owned by the current runner instead of walking all entries. Change
the loops that use globalHookStores.entries() (and the calls to
resolveHookSourceCandidate, sourceCandidates and resolveHookSourceMap) to filter
by the runner identity (e.g., entry.owner or compare entry.runnerId to the
current runner context) or accept a runnerId parameter and only examine the
matching store(s) so only hooks registered by that runner are consulted; apply
the same scoping change to the other occurrences you flagged (around the blocks
at 269-288 and 476-479).
| function rewriteModuleImportsForDataUrl(params: { | ||
| code: string; | ||
| filePath: string; | ||
| }) { | ||
| const moduleDir = path.dirname(params.filePath); | ||
| const originalFileUrl = pathToFileURL(params.filePath).href; | ||
| const importStatementRE = | ||
| /import\s+(?:[^'"`]*?\s+from\s+)?(['"])([^'"]+)\1\s*;?/g; | ||
| const rewrittenImports = params.code.replace( | ||
| importStatementRE, | ||
| (statement: string, quote: string, specifier: string) => { | ||
| if (specifier.endsWith('.css')) { | ||
| return ''; | ||
| } | ||
|
|
||
| if (!specifier.startsWith('.')) { | ||
| return statement; | ||
| } | ||
|
|
||
| const absoluteFileUrl = pathToFileURL( | ||
| path.resolve(moduleDir, specifier) | ||
| ).href; | ||
| return statement.replace( | ||
| `${quote}${specifier}${quote}`, | ||
| `${quote}${absoluteFileUrl}${quote}` | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| return rewrittenImports.replaceAll( | ||
| 'createRequire(import.meta.url)', | ||
| `createRequire(${JSON.stringify(originalFileUrl)})` | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js ESM, how are relative specifiers resolved from modules imported via data:URLs? Doexport ... from "./x.js"andimport("./x.js") resolve relative to the original file path?
💡 Result:
In Node.js ESM, all module specifiers inside a module are resolved relative to that module’s own URL (the “parentURL”), not relative to wherever the string that produced it originally came from.
So if a module is loaded from a data: URL, then:
export ... from "./x.js"does not resolve relative to the original file path.import("./x.js")does not resolve relative to the original file path.- Instead, both attempt to resolve
./x.jsagainst thedata:URL as the base URL, and that fails becausedata:URLs are non-hierarchical, so there is no meaningful “directory” to resolve./from. Node documents this explicitly (example:data:text/javascript,import "./foo";fails). [1] - In practice you’ll see errors like “Invalid URL” / “base scheme is not hierarchical” /
ERR_UNSUPPORTED_RESOLVE_REQUESTwhen trying to do relative imports from adata:parent. [2]
This matches the platform URL-based module resolution model: resolving a relative specifier like ../foo requires a hierarchical base URL, and data: can’t serve as that base. [3]
Implication: If you need the data:-loaded module to import/re-export other modules, you must use absolute specifiers (e.g., file: / node: / fully qualified URLs), or avoid data: as the parent module URL. [1]
Sources
- Node.js ESM docs (example showing relative import from
data:fails) — https://nodejs.org/download/release/v16.18.1/docs/api/esm.html [1] - Node.js issue showing the runtime error pattern when importing from a
data:parent — Importing module on filesystem from data URL throwsTypeError: Invalid URL/ERR_UNSUPPORTED_RESOLVE_REQUEST: Invalid relative URL or base scheme is not hierarchical.nodejs/node#51956 [2] - HTML / platform “resolve a module specifier” behavior with
data:as base URL — https://html.spec.whatwg.org/multipage/webappapis.html [3]
🏁 Script executed:
cat -n packages/ssr/src/build-preview.ts | sed -n '100,200p'Repository: farm-fe/farm
Length of output: 3618
Relative imports will fail when loaded from the data: URL fallback.
The rewriteModuleImportsForDataUrl regex only rewrites static import ... from statements, leaving export ... from './x.js' and import('./x.js') unresolved. When this code is loaded from a data: URL (line 171), these relative specifiers will fail to resolve because data: is non-hierarchical and cannot serve as a base for module resolution. Either rewrite all relative ESM specifiers using a parser, or load from a real file-backed URL instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/build-preview.ts` around lines 117 - 149, The regex-only
approach in rewriteModuleImportsForDataUrl leaves relative specifiers in export
... from and dynamic import(...) forms, which will break when code is loaded
from a data: URL; update rewriteModuleImportsForDataUrl so all relative ESM
specifiers are rewritten to absolute file:// URLs: either (A) parse the module
(e.g., with a lightweight parser or acorn/estree) and replace specifiers for
ImportDeclaration, ExportNamedDeclaration/ExportAllDeclaration and
ImportExpression nodes, using pathToFileURL(path.resolve(moduleDir,
specifier)).href and keep the createRequire replacement using originalFileUrl,
or (B) avoid data: URLs entirely by writing the transformed code to a temporary
file and use its file:// URL as the module entry instead of the data: URL;
adjust the code paths that call rewriteModuleImportsForDataUrl (and the
createRequire(import.meta.url) replacement) accordingly.
| function emitRunnerPayload( | ||
| emit: (payload: RunnerHotPayload) => void, | ||
| result: SsrUpdateResult | ||
| ) { | ||
| const changedModules = [ | ||
| ...result.changed, | ||
| ...result.added, | ||
| ...result.extraWatchResult.add | ||
| ]; | ||
|
|
||
| if (changedModules.length > 0) { | ||
| emit({ | ||
| type: 'update', | ||
| updates: changedModules.map((path) => ({ | ||
| type: 'js-update', | ||
| path, | ||
| acceptedPath: path, | ||
| timestamp: Date.now() | ||
| })) | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (result.removed.length > 0) { | ||
| emit({ type: 'full-reload' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't ignore removals when other modules also changed.
If result.removed is non-empty together with changed/added, Line 473 returns an update payload and never emits the full reload already required for removed modules. Rename/delete events can then leave the runner with a stale module graph.
Proposed fix
function emitRunnerPayload(
emit: (payload: RunnerHotPayload) => void,
result: SsrUpdateResult
) {
+ if (result.removed.length > 0) {
+ emit({ type: 'full-reload' });
+ return;
+ }
+
const changedModules = [
...result.changed,
...result.added,
...result.extraWatchResult.add
];
@@
- if (result.removed.length > 0) {
- emit({ type: 'full-reload' });
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function emitRunnerPayload( | |
| emit: (payload: RunnerHotPayload) => void, | |
| result: SsrUpdateResult | |
| ) { | |
| const changedModules = [ | |
| ...result.changed, | |
| ...result.added, | |
| ...result.extraWatchResult.add | |
| ]; | |
| if (changedModules.length > 0) { | |
| emit({ | |
| type: 'update', | |
| updates: changedModules.map((path) => ({ | |
| type: 'js-update', | |
| path, | |
| acceptedPath: path, | |
| timestamp: Date.now() | |
| })) | |
| }); | |
| return; | |
| } | |
| if (result.removed.length > 0) { | |
| emit({ type: 'full-reload' }); | |
| } | |
| } | |
| function emitRunnerPayload( | |
| emit: (payload: RunnerHotPayload) => void, | |
| result: SsrUpdateResult | |
| ) { | |
| if (result.removed.length > 0) { | |
| emit({ type: 'full-reload' }); | |
| return; | |
| } | |
| const changedModules = [ | |
| ...result.changed, | |
| ...result.added, | |
| ...result.extraWatchResult.add | |
| ]; | |
| if (changedModules.length > 0) { | |
| emit({ | |
| type: 'update', | |
| updates: changedModules.map((path) => ({ | |
| type: 'js-update', | |
| path, | |
| acceptedPath: path, | |
| timestamp: Date.now() | |
| })) | |
| }); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/dev-server.ts` around lines 463 - 489, The emitRunnerPayload
function currently returns an 'update' when changed/added exist and skips
'full-reload' even if result.removed is non-empty; change the control flow in
emitRunnerPayload so that if result.removed.length > 0 you emit a { type:
'full-reload' } (and return) before emitting any 'update' payloads, or otherwise
ensure the presence of removals forces a full reload; update logic around the
changedModules calculation and the early return to check result.removed first
(refer to emitRunnerPayload, changedModules, and result.removed).
| function watchServerCompilerWithSingleWatcher(params: { | ||
| watcher: SsrDevWatcherLike; | ||
| compiler: SsrDevCompilerLike; | ||
| onUpdateResult: (result: SsrUpdateResult) => void | Promise<void>; | ||
| }) { | ||
| let active = true; | ||
|
|
||
| const runUpdate = async (path: string, type: SsrUpdateType) => { | ||
| if (!active) { | ||
| return; | ||
| } | ||
|
|
||
| let result: SsrUpdateResult; | ||
| try { | ||
| result = await params.compiler.update([{ path, type }]); | ||
| } catch { | ||
| result = { | ||
| added: [], | ||
| changed: type === 'removed' ? [] : [path], | ||
| removed: type === 'removed' ? [path] : [], | ||
| extraWatchResult: { | ||
| add: [] | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| if (!active) { | ||
| return; | ||
| } | ||
|
|
||
| await params.onUpdateResult(result); | ||
| }; | ||
|
|
||
| params.watcher.on('add', async (file) => { | ||
| await runUpdate(normalizePath(file), 'added'); | ||
| }); | ||
|
|
||
| params.watcher.on('unlink', async (file) => { | ||
| await runUpdate(normalizePath(file), 'removed'); | ||
| }); | ||
|
|
||
| params.watcher.on('change', async (file) => { | ||
| const normalizedFile = normalizePath(file); | ||
| await runUpdate(normalizedFile, 'updated'); | ||
| }); |
There was a problem hiding this comment.
Serialize watcher-driven server recompiles.
These watcher callbacks run independently, but each one recreates the compiler and mutates the shared invokeContext. On bursty file changes, an older rebuild can finish after a newer one and overwrite the latest compiler/root/publicPath state. Queue or coalesce updates before calling onUpdateResult.
Sketch of a fix
function watchServerCompilerWithSingleWatcher(params: {
watcher: SsrDevWatcherLike;
compiler: SsrDevCompilerLike;
onUpdateResult: (result: SsrUpdateResult) => void | Promise<void>;
}) {
let active = true;
+ let updateChain = Promise.resolve();
@@
- params.watcher.on('add', async (file) => {
- await runUpdate(normalizePath(file), 'added');
- });
+ const enqueue = (file: string, type: SsrUpdateType) => {
+ updateChain = updateChain.then(() => runUpdate(normalizePath(file), type));
+ return updateChain.catch(() => undefined);
+ };
- params.watcher.on('unlink', async (file) => {
- await runUpdate(normalizePath(file), 'removed');
- });
+ params.watcher.on('add', (file) => enqueue(file, 'added'));
+ params.watcher.on('unlink', (file) => enqueue(file, 'removed'));
+ params.watcher.on('change', (file) => enqueue(file, 'updated'));
-
- params.watcher.on('change', async (file) => {
- const normalizedFile = normalizePath(file);
- await runUpdate(normalizedFile, 'updated');
- });Also applies to: 853-910
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/dev-server.ts` around lines 505 - 549, The watcher handlers
in watchServerCompilerWithSingleWatcher (the params.watcher.on
'add'/'unlink'/'change' callbacks and runUpdate calling params.compiler.update
and params.onUpdateResult) can run concurrently and overwrite shared
invokeContext; serialize/coalesce these updates by introducing a single async
FIFO processor: queue incoming events (path+type) from the watcher callbacks,
have a single loop/task that dequeues events and runs params.compiler.update
sequentially, and coalesce bursty events (e.g., collapse multiple events for the
same path to the latest type) before calling params.onUpdateResult so only one
update runs at a time and only the latest state is applied to the shared
invokeContext. Ensure runUpdate is adapted to enqueue rather than directly
invoke params.compiler.update and that the processor respects the active flag
and drains/cleans the queue on shutdown.
…e ModuleRunner analysis - Introduce SsrRuntime unified interface replacing SsrServer - Add SsrRuntimeHooks for lifecycle observability (compile, invalidate, render) - Implement asset manifest generation and injection for build outputs - Support render() method for both dev/preview servers - Consolidate SSR Toolkit example into single farm.config.ts - Complete Farm vs Vite ModuleRunner analysis and P0/P1 implementation roadmap - Add multi-runtime smoke tests (Node/Worker/Deno/Bun) - Enhance error handling with typed SsrError codes
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
packages/ssr/src/dev-server.ts (3)
482-507:⚠️ Potential issue | 🟠 MajorForce full reload when any module is removed.
Line 492 returns after emitting
update, so when removed + changed coexist, Line 505 is skipped and removals are ignored.Proposed fix
function emitRunnerPayload( emit: (payload: RunnerHotPayload) => void, result: SsrUpdateResult ) { + if (result.removed.length > 0) { + emit({ type: 'full-reload' }); + return; + } + const changedModules = [ ...result.changed, ...result.added, ...result.extraWatchResult.add ]; @@ - if (result.removed.length > 0) { - emit({ type: 'full-reload' }); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/dev-server.ts` around lines 482 - 507, The current emitRunnerPayload returns immediately after emitting an 'update', which skips the removal check, so removals are ignored when both changed and removed modules exist; move or add the removed-modules check so that if result.removed.length > 0 you emit a { type: 'full-reload' } (and return) before/without being short-circuited by the changedModules branch. Update the emitRunnerPayload function to check result.removed first (or always evaluate removals after computing changedModules) and ensure the full-reload is emitted whenever result.removed is non-empty, referencing the emitRunnerPayload function, changedModules variable, and result.removed.
616-640:⚠️ Potential issue | 🟠 MajorUse function replacers when injecting
appHtmlinto templates.At Line 622 and Line 637, string replacement treats
$sequences inappHtmlas replacement patterns, which can corrupt SSR output.Proposed fix
if (params.template.includes(params.placeholder)) { - return params.template.replace(params.placeholder, params.appHtml); + return params.template.replace(params.placeholder, () => params.appHtml); } @@ return params.template.replace( ROOT_CONTAINER_PATTERN, - `<div id="root">${params.appHtml}</div>` + () => `<div id="root">${params.appHtml}</div>` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/dev-server.ts` around lines 616 - 640, The injectAppHtmlToTemplate function currently uses string replacement with replacement strings (in the two calls to params.template.replace) which treats $ sequences in params.appHtml as replacement patterns and can corrupt SSR output; change both replacements to use replacer functions so the returned content is used verbatim: replace params.placeholder with a function that returns params.appHtml, and replace ROOT_CONTAINER_PATTERN with a function that returns `<div id="root">${params.appHtml}</div>`; keep the same placeholder/error logic and reference function injectAppHtmlToTemplate, params.template, params.appHtml, DEFAULT_TEMPLATE_PLACEHOLDER, and ROOT_CONTAINER_PATTERN.
524-575:⚠️ Potential issue | 🟠 MajorSerialize watcher-triggered compiler updates to avoid stale overwrite races.
The
add/unlink/changehandlers triggerrunUpdateindependently. Concurrent updates can finish out of order and overwrite shared runtime/compiler state.Proposed fix
function watchServerCompilerWithSingleWatcher(params: { @@ }) { let active = true; + let updateChain = Promise.resolve(); @@ - params.watcher.on('add', async (file) => { - await runUpdate(normalizePath(file), 'added'); - }); + const enqueueUpdate = (file: string, type: SsrUpdateType) => { + const normalizedFile = normalizePath(file); + updateChain = updateChain + .then(() => runUpdate(normalizedFile, type)) + .catch(() => undefined); + return updateChain; + }; - params.watcher.on('unlink', async (file) => { - await runUpdate(normalizePath(file), 'removed'); - }); + params.watcher.on('add', (file) => { + void enqueueUpdate(file, 'added'); + }); + params.watcher.on('unlink', (file) => { + void enqueueUpdate(file, 'removed'); + }); + params.watcher.on('change', (file) => { + void enqueueUpdate(file, 'updated'); + }); - - params.watcher.on('change', async (file) => { - const normalizedFile = normalizePath(file); - await runUpdate(normalizedFile, 'updated'); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/dev-server.ts` around lines 524 - 575, The watcher handlers in watchServerCompilerWithSingleWatcher currently call runUpdate concurrently (via params.compiler.update), causing out-of-order finishes and stale overwrites; change the implementation to serialize updates by introducing a per-watcher update queue or pending promise chain so each runUpdate waits for the previous to finish before starting, e.g., enqueue events from params.watcher.on('add'|'change'|'unlink') and process them sequentially, ensuring runUpdate (which calls params.compiler.update and then params.onUpdateResult) is never running concurrently while respecting the active flag and cancelling/clearing the queue when active becomes false.
🧹 Nitpick comments (7)
examples/ssr-toolkit/src/router.ts (1)
14-35: Avoid positional coupling betweenrouteRecordsandroutes.Using
routes[0/1/2]makes headings fragile ifroutesis reordered or extended. Prefer key-based lookup to keep metadata mapping stable.♻️ Proposed refactor
-import { notFoundRoute, routes } from './routes.mjs'; +import { getRouteByKey, notFoundRoute } from './routes.mjs'; const routeRecords: RouteRecordRaw[] = [ { path: '/', name: 'home', component: HomePage, meta: { key: 'home', - heading: routes[0].heading, + heading: getRouteByKey('home')?.heading ?? 'Home', status: 'ok' } }, { path: '/about', name: 'about', component: () => import('./pages/AboutPage.vue'), meta: { key: 'about', - heading: routes[1].heading, + heading: getRouteByKey('about')?.heading ?? 'About', status: 'ok' } }, { path: '/products', name: 'products', component: ProductsPage, meta: { key: 'products', - heading: routes[2].heading, + heading: getRouteByKey('products')?.heading ?? 'Products', status: 'ok' } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ssr-toolkit/src/router.ts` around lines 14 - 35, The route definitions currently access headings via positional indexing into the routes array (e.g., routes[0].heading, routes[1].heading, routes[2].heading), which is fragile; change them to use a key-based lookup instead by implementing a small helper (e.g., findRouteHeading or getHeadingByKey) that searches the routes array for an object whose key matches the meta.key and returns its heading, then replace all occurrences of routes[0/1/2].heading in the route records (the entries for path '/', '/about', '/products' and their meta objects) with calls to that helper; ensure the helper handles missing keys (fallback or undefined) and reference the existing meta.key property on each route to perform the lookup.packages/ssr/src/command.ts (1)
59-61: Use destructured variable instead of re-accessing property.Line 61 accesses
options.startbut the value was already destructured to_starton line 59. While functionally equivalent, using the destructured variable is more consistent.♻️ Proposed fix
const { start: _start, ...runtimeOptions } = options as SsrRunServerCommandOptions; - const shouldStart = options.start ?? true; + const shouldStart = _start ?? true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/command.ts` around lines 59 - 61, The code destructures start into _start from options but then re-reads options.start; change the shouldStart assignment to use the destructured _start (i.e., set shouldStart using _start ?? true) so you consistently use the extracted variable from SsrRunServerCommandOptions rather than re-accessing options.start; update the reference in the shouldStart declaration to _start.packages/ssr/src/config-resolver.ts (2)
17-19: Unusedcommandparameter.The
commandparameter is declared but never used in the function body. If this is intentional for future use or API consistency, consider prefixing with underscore (_command) to signal intent.♻️ Proposed fix
export function resolveSsrConfigForCommand<T extends SsrConfig>( config: T, - command: 'dev' | 'preview' | 'build' + _command: 'dev' | 'preview' | 'build' ): {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/config-resolver.ts` around lines 17 - 19, The function resolveSsrConfigForCommand declares a command parameter that's unused; either use it or mark intent by renaming to _command to avoid lint warnings and communicate it's intentionally unused. Update the function signature (resolveSsrConfigForCommand<T extends SsrConfig>(config: T, _command: 'dev' | 'preview' | 'build')) or incorporate the command into the function logic where appropriate, ensuring references to the parameter inside the body match the new name if renamed.
39-49: Simplify redundant server resolution logic.Both branches (
config.servertruthy andconfig.$servertruthy) produce identical merge results. The conditional can be simplified.♻️ Proposed fix
- const resolvedServer = config.server - ? { - ...baseServer, - ...overrideServer - } - : config.$server - ? { - ...baseServer, - ...overrideServer - } - : undefined; + const resolvedServer = + config.server || config.$server + ? { + ...baseServer, + ...overrideServer + } + : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/config-resolver.ts` around lines 39 - 49, The server resolution duplicates the same merge for both config.server and config.$server; replace the ternary with a single check that tests either property and returns the merged object (e.g., use (config.server || config.$server) to decide) so resolvedServer is either { ...baseServer, ...overrideServer } when either exists or undefined otherwise; update the expression that defines resolvedServer accordingly (references: resolvedServer, config.server, config.$server, baseServer, overrideServer).packages/ssr/src/runtime.ts (1)
50-75: Inconsistent server config handling between preview and dev paths.In the preview path (Lines 58-61),
serveris always created with a fallback to empty object (resolved.server ?? {}), ensuring it's always present. In the dev path (Lines 84-91),serveris conditionally spread only whenresolved.serverexists.This asymmetry could lead to different behavior depending on the command. If preview requires a server config to always exist, consider documenting this requirement or aligning the dev path behavior.
Also applies to: 77-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/runtime.ts` around lines 50 - 75, The preview path always ensures a server config exists by using ...(resolved.server ?? {}) and defaulting server.mode to resolved.server?.mode ?? mode; make the dev path build its options the same way: when constructing the dev options (the block analogous to previewOptions in the dev command), spread ...(resolved.server ?? {}) into server and set server.mode to resolved.server?.mode ?? mode so both commands behave consistently; update the dev options construction where resolveSsrConfigForCommand(resolved, ...) is used to match the previewOptions pattern.packages/ssr/src/server.ts (1)
62-100: Consider extracting duplicated command/mode resolution.Both
toDevServerOptionsandtoPreviewServerOptionscallresolveCommandandresolveModeidentically. Consider extracting common resolution to reduce duplication.♻️ Proposed refactor
+function resolveOptionsBase(options: SsrServerOptions) { + const command = resolveCommand(options); + const mode = resolveMode(options, command); + const resolved = resolveSsrConfigForCommand(options, command); + return { command, mode, resolved }; +} + function toDevServerOptions(options: SsrServerOptions): SsrDevServerOptions { - const command = resolveCommand(options); - const mode = resolveMode(options, command); - const resolved = resolveSsrConfigForCommand(options, command); + const { mode, resolved } = resolveOptionsBase(options); return { ...resolved, client: { ...resolved.client, mode: resolved.client.mode ?? mode }, ...(resolved.server ? { server: { ...resolved.server, mode: resolved.server.mode ?? mode } } : {}) }; } function toPreviewServerOptions(options: SsrServerOptions): SsrPreviewOptions { - const command = resolveCommand(options); - const mode = resolveMode(options, command); - const resolved = resolveSsrConfigForCommand(options, command); + const { mode, resolved } = resolveOptionsBase(options); return { ...resolved, client: { ...resolved.client, mode: resolved.client.mode ?? mode }, server: { ...(resolved.server ?? {}), mode: resolved.server?.mode ?? mode } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/src/server.ts` around lines 62 - 100, Extract the repeated resolveCommand/resolveMode/resolveSsrConfigForCommand logic into a small helper (e.g., getResolvedSsrOptions(options: SsrServerOptions) returning { command, mode, resolved }) and call it from both toDevServerOptions and toPreviewServerOptions; replace the three duplicated lines in each function (the resolveCommand, resolveMode, and resolveSsrConfigForCommand calls) with a single call to that helper and use its returned command/mode/resolved values to build the returned object, keeping existing behavior for client.mode and server.mode defaults.packages/ssr/test/build-preview.spec.ts (1)
831-872: Integration tests use real filesystem - ensure cleanup on failure.The CSS interop tests correctly use
try/finallyto clean up temp directories, which is good practice. However, ifmkdtempsucceeds butwriteFilefails, the directory may be left behind.Consider wrapping the entire test body in the try block:
♻️ Proposed improvement
it('imports server module with css side-effect imports via fallback interop', async () => { const tmpRoot = await mkdtemp(path.join(tmpdir(), 'farm-ssr-css-interop-')); - const entryPath = path.join(tmpRoot, 'entry.mjs'); - const depPath = path.join(tmpRoot, 'dep.mjs'); - const cssPath = path.join(tmpRoot, 'style.css'); try { + const entryPath = path.join(tmpRoot, 'entry.mjs'); + const depPath = path.join(tmpRoot, 'dep.mjs'); + const cssPath = path.join(tmpRoot, 'style.css'); + await writeFile(depPath, "export const value = 'ok';\n", 'utf-8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ssr/test/build-preview.spec.ts` around lines 831 - 872, Move the temporary directory creation inside the try block (or guard the finally cleanup with a check that tmpRoot is truthy) so we always attempt removal only if mkdtemp succeeded; specifically, in these tests reference the tmpRoot variable created by mkdtemp and ensure the try/finally wraps the mkdtemp + writeFile + importModuleWithCssInterop sequence (or change finally to call rm only when tmpRoot is defined) so rm(tmpRoot, { recursive: true, force: true }) never runs on an undefined path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/ssr.ts`:
- Around line 258-264: buildFarmConfig currently copies options.input into every
config which causes --input to overwrite the server render entry; update the
logic so that when creating the server bundle config (and in
resolveSsrRunOptions when producing server run options) you do NOT apply
options.input to the server side. Concretely, ensure buildFarmConfig and
resolveSsrRunOptions only attach options.input to the client config/path (or set
input to undefined/omit it) for the server config, and update the sections
referenced (around buildFarmConfig, the resolveSsrRunOptions usage at lines
~315-320 and ~357-370) so the server bundle preserves its original entry without
being replaced by --input.
- Around line 45-52: The resolveConfigFile helper currently forces a fallback
path (params.fallback) which causes explicit injection of farm.config.client.ts
/ farm.config.server.ts even when no flags are passed; change resolveConfigFile
to allow returning undefined when neither dedicatedConfigFile nor
sharedConfigFile are provided (make fallback optional: fallback?: string and
return params.dedicatedConfigFile ?? params.sharedConfigFile ?? undefined) and
update all call sites that currently pass a hardcoded fallback to only pass it
when the user explicitly supplied a config flag, so the normal farm.config.*
discovery (including the examples/ssr-toolkit shared config) can run; update
callers referenced around the other occurrences noted (lines ~258-264, ~301-304,
~343-352) to stop forcing the fallback.
- Around line 33-39: DEFAULT_DEV_ENTRY_CANDIDATES are currently absolute (start
with '/') which causes toAbsolutePath(path) to treat them as already-absolute
and makes existsSync probe the filesystem root; change the entries to relative
forms (e.g., './src/entry-server.mjs', './src/entry-server.ts',
'./src/entry-server.js', './src/entry-server.tsx', './src/entry-server.jsx')
wherever DEFAULT_DEV_ENTRY_CANDIDATES is defined so path.isAbsolute() returns
false and toAbsolutePath(root, candidate) will join with the project root for
existsSync checks; also ensure the code that returns the chosen entry (the value
produced from the candidate after checking existsSync) returns the
project-relative candidate (strip the './' prefix if necessary) rather than the
absolute path returned by toAbsolutePath — update uses around
DEFAULT_DEV_ENTRY_CANDIDATES and the selection logic that calls toAbsolutePath
and existsSync accordingly.
In `@packages/ssr/src/dev-server.ts`:
- Around line 74-78: The middleware type and runner must handle async
middleware: change the SsrMiddleware typedef to return void | Promise<void>
(i.e., export type SsrMiddleware = (... ) => void | Promise<void>) and update
the middleware runner in createMiddlewareServer to capture the middleware return
value, detect a Promise (e.g., result && typeof result.then === 'function'), and
attach a .catch(err => next?.(err) || handleError(err)) so rejected Promises
invoke the existing error path instead of becoming unhandled rejections; apply
the same pattern to the other middleware invocation site mentioned in the
comment.
In `@packages/ssr/src/manifest.ts`:
- Around line 126-136: The asset paths in the loops that push into
headLinks/bodyScripts (the iterations over params.assets.css,
params.assets.preload, and params.assets.scripts) are interpolated directly into
HTML attributes and must be sanitized or validated to prevent XSS; update the
code that constructs these links (the headLinks.push and bodyScripts.push sites)
to either HTML-escape special characters in href/src (at minimum &, <, >, ", and
') or validate each path against a strict whitelist/regex (e.g., must start with
'/' or 'https://' and contain no quotes or control characters) and skip or error
on invalid values before interpolation. Ensure the chosen escape/validation
function is applied consistently to href in the css/preload loops and to src in
the scripts loop.
---
Duplicate comments:
In `@packages/ssr/src/dev-server.ts`:
- Around line 482-507: The current emitRunnerPayload returns immediately after
emitting an 'update', which skips the removal check, so removals are ignored
when both changed and removed modules exist; move or add the removed-modules
check so that if result.removed.length > 0 you emit a { type: 'full-reload' }
(and return) before/without being short-circuited by the changedModules branch.
Update the emitRunnerPayload function to check result.removed first (or always
evaluate removals after computing changedModules) and ensure the full-reload is
emitted whenever result.removed is non-empty, referencing the emitRunnerPayload
function, changedModules variable, and result.removed.
- Around line 616-640: The injectAppHtmlToTemplate function currently uses
string replacement with replacement strings (in the two calls to
params.template.replace) which treats $ sequences in params.appHtml as
replacement patterns and can corrupt SSR output; change both replacements to use
replacer functions so the returned content is used verbatim: replace
params.placeholder with a function that returns params.appHtml, and replace
ROOT_CONTAINER_PATTERN with a function that returns `<div
id="root">${params.appHtml}</div>`; keep the same placeholder/error logic and
reference function injectAppHtmlToTemplate, params.template, params.appHtml,
DEFAULT_TEMPLATE_PLACEHOLDER, and ROOT_CONTAINER_PATTERN.
- Around line 524-575: The watcher handlers in
watchServerCompilerWithSingleWatcher currently call runUpdate concurrently (via
params.compiler.update), causing out-of-order finishes and stale overwrites;
change the implementation to serialize updates by introducing a per-watcher
update queue or pending promise chain so each runUpdate waits for the previous
to finish before starting, e.g., enqueue events from
params.watcher.on('add'|'change'|'unlink') and process them sequentially,
ensuring runUpdate (which calls params.compiler.update and then
params.onUpdateResult) is never running concurrently while respecting the active
flag and cancelling/clearing the queue when active becomes false.
---
Nitpick comments:
In `@examples/ssr-toolkit/src/router.ts`:
- Around line 14-35: The route definitions currently access headings via
positional indexing into the routes array (e.g., routes[0].heading,
routes[1].heading, routes[2].heading), which is fragile; change them to use a
key-based lookup instead by implementing a small helper (e.g., findRouteHeading
or getHeadingByKey) that searches the routes array for an object whose key
matches the meta.key and returns its heading, then replace all occurrences of
routes[0/1/2].heading in the route records (the entries for path '/', '/about',
'/products' and their meta objects) with calls to that helper; ensure the helper
handles missing keys (fallback or undefined) and reference the existing meta.key
property on each route to perform the lookup.
In `@packages/ssr/src/command.ts`:
- Around line 59-61: The code destructures start into _start from options but
then re-reads options.start; change the shouldStart assignment to use the
destructured _start (i.e., set shouldStart using _start ?? true) so you
consistently use the extracted variable from SsrRunServerCommandOptions rather
than re-accessing options.start; update the reference in the shouldStart
declaration to _start.
In `@packages/ssr/src/config-resolver.ts`:
- Around line 17-19: The function resolveSsrConfigForCommand declares a command
parameter that's unused; either use it or mark intent by renaming to _command to
avoid lint warnings and communicate it's intentionally unused. Update the
function signature (resolveSsrConfigForCommand<T extends SsrConfig>(config: T,
_command: 'dev' | 'preview' | 'build')) or incorporate the command into the
function logic where appropriate, ensuring references to the parameter inside
the body match the new name if renamed.
- Around line 39-49: The server resolution duplicates the same merge for both
config.server and config.$server; replace the ternary with a single check that
tests either property and returns the merged object (e.g., use (config.server ||
config.$server) to decide) so resolvedServer is either { ...baseServer,
...overrideServer } when either exists or undefined otherwise; update the
expression that defines resolvedServer accordingly (references: resolvedServer,
config.server, config.$server, baseServer, overrideServer).
In `@packages/ssr/src/runtime.ts`:
- Around line 50-75: The preview path always ensures a server config exists by
using ...(resolved.server ?? {}) and defaulting server.mode to
resolved.server?.mode ?? mode; make the dev path build its options the same way:
when constructing the dev options (the block analogous to previewOptions in the
dev command), spread ...(resolved.server ?? {}) into server and set server.mode
to resolved.server?.mode ?? mode so both commands behave consistently; update
the dev options construction where resolveSsrConfigForCommand(resolved, ...) is
used to match the previewOptions pattern.
In `@packages/ssr/src/server.ts`:
- Around line 62-100: Extract the repeated
resolveCommand/resolveMode/resolveSsrConfigForCommand logic into a small helper
(e.g., getResolvedSsrOptions(options: SsrServerOptions) returning { command,
mode, resolved }) and call it from both toDevServerOptions and
toPreviewServerOptions; replace the three duplicated lines in each function (the
resolveCommand, resolveMode, and resolveSsrConfigForCommand calls) with a single
call to that helper and use its returned command/mode/resolved values to build
the returned object, keeping existing behavior for client.mode and server.mode
defaults.
In `@packages/ssr/test/build-preview.spec.ts`:
- Around line 831-872: Move the temporary directory creation inside the try
block (or guard the finally cleanup with a check that tmpRoot is truthy) so we
always attempt removal only if mkdtemp succeeded; specifically, in these tests
reference the tmpRoot variable created by mkdtemp and ensure the try/finally
wraps the mkdtemp + writeFile + importModuleWithCssInterop sequence (or change
finally to call rm only when tmpRoot is defined) so rm(tmpRoot, { recursive:
true, force: true }) never runs on an undefined path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 720245a1-c7ac-4328-88b3-0fdf70b73ac9
📒 Files selected for processing (30)
examples/ssr-toolkit/README.mdexamples/ssr-toolkit/farm.config.tsexamples/ssr-toolkit/package.jsonexamples/ssr-toolkit/server.mjsexamples/ssr-toolkit/server/ssr-options.mjsexamples/ssr-toolkit/src/pages/AboutPage.vueexamples/ssr-toolkit/src/pages/HomePage.vueexamples/ssr-toolkit/src/pages/ProductsPage.vueexamples/ssr-toolkit/src/router.tsexamples/ssr-toolkit/src/routes.mjsexamples/ssr-toolkit/tsconfig.jsonpackages/cli/src/ssr.spec.tspackages/cli/src/ssr.tspackages/ssr/src/build-preview.tspackages/ssr/src/command.tspackages/ssr/src/config-resolver.tspackages/ssr/src/dev-server.tspackages/ssr/src/errors.tspackages/ssr/src/index.tspackages/ssr/src/manifest.tspackages/ssr/src/runtime-hooks.tspackages/ssr/src/runtime-types.tspackages/ssr/src/runtime.tspackages/ssr/src/server.tspackages/ssr/test/build-preview.spec.tspackages/ssr/test/command.spec.tspackages/ssr/test/dev-server.spec.tspackages/ssr/test/manifest.spec.tspackages/ssr/test/ssr-toolkit-example.spec.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (9)
- examples/ssr-toolkit/tsconfig.json
- vitest.config.ts
- examples/ssr-toolkit/src/pages/ProductsPage.vue
- examples/ssr-toolkit/package.json
- examples/ssr-toolkit/src/pages/AboutPage.vue
- packages/ssr/src/runtime-types.ts
- packages/ssr/test/manifest.spec.ts
- examples/ssr-toolkit/src/pages/HomePage.vue
- examples/ssr-toolkit/server.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/ssr/test/command.spec.ts
- packages/cli/src/ssr.spec.ts
- packages/ssr/test/ssr-toolkit-example.spec.ts
- packages/ssr/test/dev-server.spec.ts
- examples/ssr-toolkit/README.md
- packages/ssr/src/index.ts
- packages/ssr/src/build-preview.ts
| const DEFAULT_DEV_ENTRY_CANDIDATES = [ | ||
| '/src/entry-server.mjs', | ||
| '/src/entry-server.ts', | ||
| '/src/entry-server.js', | ||
| '/src/entry-server.tsx', | ||
| '/src/entry-server.jsx' | ||
| ] as const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
node - <<'NODE'
const path = require('node:path');
for (const candidate of ['/src/entry-server.ts', './src/entry-server.ts']) {
console.log(JSON.stringify({
candidate,
isAbsolute: path.isAbsolute(candidate)
}));
}
NODERepository: farm-fe/farm
Length of output: 167
🏁 Script executed:
wc -l packages/cli/src/ssr.tsRepository: farm-fe/farm
Length of output: 83
🏁 Script executed:
cat -n packages/cli/src/ssr.ts | head -200Repository: farm-fe/farm
Length of output: 6038
🏁 Script executed:
# Check the toAbsolutePath helper around lines 124-129
cat -n packages/cli/src/ssr.ts | sed -n '115,140p'Repository: farm-fe/farm
Length of output: 721
🏁 Script executed:
# Check resolveEntry usage around line 155 and resolveBuildEntry around line 176
cat -n packages/cli/src/ssr.ts | sed -n '145,190p'Repository: farm-fe/farm
Length of output: 1351
🏁 Script executed:
# Look for where these resolved entries are actually used
rg -n "resolveEntry|resolveBuildEntry" packages/cli/src/ssr.ts -A 5 -B 2Repository: farm-fe/farm
Length of output: 842
🏁 Script executed:
# Check where the returned entry is used in the config
cat -n packages/cli/src/ssr.ts | sed -n '210,230p'Repository: farm-fe/farm
Length of output: 672
🏁 Script executed:
# Also check line 389 area where resolveEntry result is used
cat -n packages/cli/src/ssr.ts | sed -n '380,400p'Repository: farm-fe/farm
Length of output: 601
Fix the default entry candidates; they currently resolve from filesystem root.
DEFAULT_DEV_ENTRY_CANDIDATES starts with /, and toAbsolutePath() checks path.isAbsolute() before stripping that prefix. On Node.js, /src/entry-server.ts is absolute, so toAbsolutePath() returns it unchanged. The existsSync() checks on lines 155 and 176 then probe /src/... on the filesystem root instead of <root>/src/.... The returned entry value is also incorrect—it remains an absolute path like /src/entry-server.ts instead of a project-relative path. This breaks SSR entry discovery and build configuration.
Change the candidates to relative paths (e.g., ./src/entry-server.mjs) so path.isAbsolute() returns false and toAbsolutePath() properly joins them with the project root.
Proposed fix
const DEFAULT_DEV_ENTRY_CANDIDATES = [
- '/src/entry-server.mjs',
- '/src/entry-server.ts',
- '/src/entry-server.js',
- '/src/entry-server.tsx',
- '/src/entry-server.jsx'
+ './src/entry-server.mjs',
+ './src/entry-server.ts',
+ './src/entry-server.js',
+ './src/entry-server.tsx',
+ './src/entry-server.jsx'
] as const;Also affects: 124–129, 154–160, 175–181
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DEFAULT_DEV_ENTRY_CANDIDATES = [ | |
| '/src/entry-server.mjs', | |
| '/src/entry-server.ts', | |
| '/src/entry-server.js', | |
| '/src/entry-server.tsx', | |
| '/src/entry-server.jsx' | |
| ] as const; | |
| const DEFAULT_DEV_ENTRY_CANDIDATES = [ | |
| './src/entry-server.mjs', | |
| './src/entry-server.ts', | |
| './src/entry-server.js', | |
| './src/entry-server.tsx', | |
| './src/entry-server.jsx' | |
| ] as const; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ssr.ts` around lines 33 - 39, DEFAULT_DEV_ENTRY_CANDIDATES
are currently absolute (start with '/') which causes toAbsolutePath(path) to
treat them as already-absolute and makes existsSync probe the filesystem root;
change the entries to relative forms (e.g., './src/entry-server.mjs',
'./src/entry-server.ts', './src/entry-server.js', './src/entry-server.tsx',
'./src/entry-server.jsx') wherever DEFAULT_DEV_ENTRY_CANDIDATES is defined so
path.isAbsolute() returns false and toAbsolutePath(root, candidate) will join
with the project root for existsSync checks; also ensure the code that returns
the chosen entry (the value produced from the candidate after checking
existsSync) returns the project-relative candidate (strip the './' prefix if
necessary) rather than the absolute path returned by toAbsolutePath — update
uses around DEFAULT_DEV_ENTRY_CANDIDATES and the selection logic that calls
toAbsolutePath and existsSync accordingly.
| function resolveConfigFile(params: { | ||
| sharedConfigFile?: string; | ||
| dedicatedConfigFile?: string; | ||
| fallback: string; | ||
| }) { | ||
| return ( | ||
| params.dedicatedConfigFile ?? params.sharedConfigFile ?? params.fallback | ||
| ); |
There was a problem hiding this comment.
Let SSR CLI fall back to shared/default config discovery.
When no config flag is passed, this still injects farm.config.client.ts and farm.config.server.ts into both resolved configs. That bypasses the normal farm.config.* lookup and misses the new shared example config in examples/ssr-toolkit/farm.config.ts, so farm ssr ... won't pick up the Vue/Less setup out of the box.
🧭 Proposed fix
function resolveConfigFile(params: {
sharedConfigFile?: string;
dedicatedConfigFile?: string;
- fallback: string;
}) {
- return (
- params.dedicatedConfigFile ?? params.sharedConfigFile ?? params.fallback
- );
+ return params.dedicatedConfigFile ?? params.sharedConfigFile;
}
@@
function buildFarmConfig(params: {
root: string;
mode?: string;
clearScreen?: boolean;
- configFile: string;
+ configFile?: string;
options: GlobalCliOptions & CliSsrOptions;
}) {
@@
return {
root: params.root,
- configFile: params.configFile,
+ ...(params.configFile ? { configFile: params.configFile } : {}),
mode: params.mode,
clearScreen: params.clearScreen,Also applies to: 258-264, 301-304, 343-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ssr.ts` around lines 45 - 52, The resolveConfigFile helper
currently forces a fallback path (params.fallback) which causes explicit
injection of farm.config.client.ts / farm.config.server.ts even when no flags
are passed; change resolveConfigFile to allow returning undefined when neither
dedicatedConfigFile nor sharedConfigFile are provided (make fallback optional:
fallback?: string and return params.dedicatedConfigFile ??
params.sharedConfigFile ?? undefined) and update all call sites that currently
pass a hardcoded fallback to only pass it when the user explicitly supplied a
config flag, so the normal farm.config.* discovery (including the
examples/ssr-toolkit shared config) can run; update callers referenced around
the other occurrences noted (lines ~258-264, ~301-304, ~343-352) to stop forcing
the fallback.
| function buildFarmConfig(params: { | ||
| root: string; | ||
| mode?: string; | ||
| clearScreen?: boolean; | ||
| configFile: string; | ||
| options: GlobalCliOptions & CliSsrOptions; | ||
| }) { |
There was a problem hiding this comment.
Don't apply --input to the server bundle.
buildFarmConfig() copies options.input into every config it builds, and resolveSsrRunOptions() reuses it for both client and server. For farm ssr build, the CLI exposes --input but not --entry, so passing --input silently replaces the server render entry as well.
🛠️ Proposed fix
function buildFarmConfig(params: {
root: string;
mode?: string;
clearScreen?: boolean;
configFile: string;
options: GlobalCliOptions & CliSsrOptions;
+ kind: 'client' | 'server';
}) {
@@
- ...(params.options.input
+ ...(params.kind === 'client' && params.options.input
? {
input: {
index: params.options.input
@@
client: buildFarmConfig({
root,
mode: params.options.mode,
clearScreen: params.options.clearScreen,
configFile: clientConfigFile,
- options: params.options
+ options: params.options,
+ kind: 'client'
}),
server: buildFarmConfig({
root,
mode: params.options.mode,
clearScreen: params.options.clearScreen,
configFile: serverConfigFile,
- options: params.options
+ options: params.options,
+ kind: 'server'
}),Also applies to: 315-320, 357-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ssr.ts` around lines 258 - 264, buildFarmConfig currently
copies options.input into every config which causes --input to overwrite the
server render entry; update the logic so that when creating the server bundle
config (and in resolveSsrRunOptions when producing server run options) you do
NOT apply options.input to the server side. Concretely, ensure buildFarmConfig
and resolveSsrRunOptions only attach options.input to the client config/path (or
set input to undefined/omit it) for the server config, and update the sections
referenced (around buildFarmConfig, the resolveSsrRunOptions usage at lines
~315-320 and ~357-370) so the server bundle preserves its original entry without
being replaced by --input.
| export type SsrMiddleware = ( | ||
| req: IncomingMessage, | ||
| res: ServerResponse, | ||
| next?: (error?: unknown) => void | ||
| ) => void; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me check the file exists and read the specific lines
wc -l packages/ssr/src/dev-server.tsRepository: farm-fe/farm
Length of output: 91
🏁 Script executed:
# Read the middleware type definition at lines 74-78
sed -n '70,85p' packages/ssr/src/dev-server.tsRepository: farm-fe/farm
Length of output: 397
🏁 Script executed:
# Read the middleware invocation code at lines 245-275
sed -n '240,280p' packages/ssr/src/dev-server.tsRepository: farm-fe/farm
Length of output: 785
🏁 Script executed:
# Search for middleware implementations to see if they use Promises
rg "SsrMiddleware|middleware.*async|middleware.*Promise" packages/ssr/src/dev-server.ts -A 3 -B 1Repository: farm-fe/farm
Length of output: 1094
🏁 Script executed:
# Search for middleware implementations and async usage
rg "middlewares\.use|\.use\(" packages/ssr -A 5 | head -50Repository: farm-fe/farm
Length of output: 2450
🏁 Script executed:
# Search for async middleware implementations
rg "async.*\(" packages/ssr/src/dev-server.ts -B 2 -A 3Repository: farm-fe/farm
Length of output: 2903
🏁 Script executed:
# Check if any middleware in the project uses async/Promise patterns
rg "middleware.*=.*async|SsrMiddleware.*async" packages/ssr -A 3Repository: farm-fe/farm
Length of output: 38
Handle async middleware rejections in the middleware runner.
createMiddlewareServer only catches synchronous exceptions. The SsrMiddleware type declares return void, but TypeScript permits assigning async functions to this type, which would cause Promise rejections to bypass the error handler and surface as unhandled rejections.
Update the type definition to explicitly allow Promise<void> returns and add Promise rejection handling:
Proposed fix
-export type SsrMiddleware = (
+export type SsrMiddleware = (
req: IncomingMessage,
res: ServerResponse,
next?: (error?: unknown) => void
-) => void;
+) => void | Promise<void>;
@@
try {
- middleware(req, res, run);
+ const maybePromise = middleware(req, res, run);
+ if (
+ maybePromise &&
+ typeof (maybePromise as Promise<void>).then === 'function'
+ ) {
+ void (maybePromise as Promise<void>).catch((err) => run(err));
+ }
} catch (err) {
run(err);
}Also applies to: 245-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/dev-server.ts` around lines 74 - 78, The middleware type and
runner must handle async middleware: change the SsrMiddleware typedef to return
void | Promise<void> (i.e., export type SsrMiddleware = (... ) => void |
Promise<void>) and update the middleware runner in createMiddlewareServer to
capture the middleware return value, detect a Promise (e.g., result && typeof
result.then === 'function'), and attach a .catch(err => next?.(err) ||
handleError(err)) so rejected Promises invoke the existing error path instead of
becoming unhandled rejections; apply the same pattern to the other middleware
invocation site mentioned in the comment.
| for (const href of params.assets.css) { | ||
| headLinks.push(`<link rel="stylesheet" href="${href}">`); | ||
| } | ||
|
|
||
| for (const href of params.assets.preload) { | ||
| headLinks.push(`<link rel="modulepreload" href="${href}">`); | ||
| } | ||
|
|
||
| for (const src of params.assets.scripts) { | ||
| bodyScripts.push(`<script type="module" src="${src}"></script>`); | ||
| } |
There was a problem hiding this comment.
Unsanitized asset paths in HTML injection.
Asset paths from the manifest are directly interpolated into HTML attributes without escaping. If an attacker can influence the manifest (e.g., through a compromised build pipeline), this could lead to XSS via paths like " onload="alert(1).
Consider escaping special HTML characters in href and src values, or validate that paths match expected patterns (e.g., start with / and contain no quotes).
🛡️ Proposed fix with basic escaping
+function escapeHtmlAttr(value: string): string {
+ return value.replace(/&/g, '&').replace(/"/g, '"');
+}
+
export function injectAssetsIntoHtml(params: {
html: string;
assets: SsrRuntimeAssets;
}): string {
const headLinks: string[] = [];
const bodyScripts: string[] = [];
for (const href of params.assets.css) {
- headLinks.push(`<link rel="stylesheet" href="${href}">`);
+ headLinks.push(`<link rel="stylesheet" href="${escapeHtmlAttr(href)}">`);
}
for (const href of params.assets.preload) {
- headLinks.push(`<link rel="modulepreload" href="${href}">`);
+ headLinks.push(`<link rel="modulepreload" href="${escapeHtmlAttr(href)}">`);
}
for (const src of params.assets.scripts) {
- bodyScripts.push(`<script type="module" src="${src}"></script>`);
+ bodyScripts.push(`<script type="module" src="${escapeHtmlAttr(src)}"></script>`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const href of params.assets.css) { | |
| headLinks.push(`<link rel="stylesheet" href="${href}">`); | |
| } | |
| for (const href of params.assets.preload) { | |
| headLinks.push(`<link rel="modulepreload" href="${href}">`); | |
| } | |
| for (const src of params.assets.scripts) { | |
| bodyScripts.push(`<script type="module" src="${src}"></script>`); | |
| } | |
| function escapeHtmlAttr(value: string): string { | |
| return value.replace(/&/g, '&').replace(/"/g, '"'); | |
| } | |
| export function injectAssetsIntoHtml(params: { | |
| html: string; | |
| assets: SsrRuntimeAssets; | |
| }): string { | |
| const headLinks: string[] = []; | |
| const bodyScripts: string[] = []; | |
| for (const href of params.assets.css) { | |
| headLinks.push(`<link rel="stylesheet" href="${escapeHtmlAttr(href)}">`); | |
| } | |
| for (const href of params.assets.preload) { | |
| headLinks.push(`<link rel="modulepreload" href="${escapeHtmlAttr(href)}">`); | |
| } | |
| for (const src of params.assets.scripts) { | |
| bodyScripts.push(`<script type="module" src="${escapeHtmlAttr(src)}"></script>`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ssr/src/manifest.ts` around lines 126 - 136, The asset paths in the
loops that push into headLinks/bodyScripts (the iterations over
params.assets.css, params.assets.preload, and params.assets.scripts) are
interpolated directly into HTML attributes and must be sanitized or validated to
prevent XSS; update the code that constructs these links (the headLinks.push and
bodyScripts.push sites) to either HTML-escape special characters in href/src (at
minimum &, <, >, ", and ') or validate each path against a strict
whitelist/regex (e.g., must start with '/' or 'https://' and contain no quotes
or control characters) and skip or error on invalid values before interpolation.
Ensure the chosen escape/validation function is applied consistently to href in
the css/preload loops and to src in the scripts loop.
1 participant
Summary
dev/build/preview+ middleware embedding path.FARM-BUG-008(updateModulesreturn shape mismatch).middleware-mode,ssr-toolkit) for integration usage.Scope
crates/compiler,crates/noderuntime bridge updatespackages/coremodule-runner and dev update path updatespackages/ssrnew SSR toolkit package and testspackages/cliSSR command integrationexamples/middleware-modeandexamples/ssr-toolkitFeature Highlights
farm ssr dev/build/preview).Bug Fixes
vite-plugin-adapterupdateModulesreturn contract fromstring[]to tuple list[moduleId, updateType][].removed > updated > added.packages/core/src/plugin/js/vite-plugin-adapter.spec.tsValidation
pnpm --filter @farmfe/core type-checkpnpm vitest run -c vitest.config.ts packages/core/src/plugin/js/vite-plugin-adapter.spec.ts packages/core/tests/module-runner.spec.tspnpm vitest run -c vitest.config.ts packages/ssr/test/build-preview.spec.ts packages/ssr/test/dev-server.spec.ts packages/ssr/test/server.spec.ts packages/ssr/test/ssr-toolkit-example.spec.ts packages/ssr/test/command.spec.ts packages/cli/src/argv.spec.ts packages/cli/src/ssr.spec.tsSummary by CodeRabbit
Release Notes
New Features
ssr dev,ssr build, andssr previewCLI commandsBug Fixes