-
Notifications
You must be signed in to change notification settings - Fork 324
fix: warm server action modules in dev #995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| import { glob, readFile } from "node:fs/promises"; | ||
| import path from "node:path"; | ||
| import { normalizePageExtensions } from "../routing/file-matcher.js"; | ||
|
|
||
| const SERVER_ACTION_SOURCE_EXTENSIONS = ["js", "jsx", "ts", "tsx", "mjs", "mts", "cjs", "cts"]; | ||
| const SERVER_ACTION_SCAN_EXCLUDED_ROOTS = new Set([ | ||
| ".git", | ||
| ".next", | ||
| ".output", | ||
| ".refs", | ||
| ".turbo", | ||
| ".vinext", | ||
| ".worktrees", | ||
| "build", | ||
| "coverage", | ||
| "dist", | ||
| "node_modules", | ||
| "out", | ||
| ]); | ||
|
|
||
| type CollectServerActionWarmupEntriesOptions = { | ||
| root: string; | ||
| pageExtensions?: readonly string[] | null; | ||
| }; | ||
|
|
||
| function buildExtensionGlob(extensions: readonly string[]): string { | ||
| return extensions.length === 1 ? extensions[0] : `{${extensions.join(",")}}`; | ||
| } | ||
|
|
||
| function toViteEntry(root: string, filePath: string): string { | ||
| return path.relative(root, filePath).split(path.sep).join("/"); | ||
| } | ||
|
|
||
| function normalizeServerActionExtensions(pageExtensions?: readonly string[] | null): string[] { | ||
| return [ | ||
| ...new Set([...SERVER_ACTION_SOURCE_EXTENSIONS, ...normalizePageExtensions(pageExtensions)]), | ||
| ]; | ||
| } | ||
|
|
||
| function shouldExcludeServerActionScanPath(name: string): boolean { | ||
| const segments = name.split(/[\\/]+/).filter(Boolean); | ||
| return ( | ||
| SERVER_ACTION_SCAN_EXCLUDED_ROOTS.has(segments[0] ?? "") || segments.includes("node_modules") | ||
| ); | ||
| } | ||
|
|
||
| function skipWhitespaceAndComments(source: string, start: number): number { | ||
| let index = start; | ||
| while (index < source.length) { | ||
| const char = source[index]; | ||
| const next = source[index + 1]; | ||
|
|
||
| if ( | ||
| char === " " || | ||
| char === "\t" || | ||
| char === "\n" || | ||
| char === "\r" || | ||
| char === "\f" || | ||
| char === "\v" | ||
| ) { | ||
| index++; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === "/" && next === "/") { | ||
| index += 2; | ||
| while (index < source.length && source[index] !== "\n" && source[index] !== "\r") { | ||
| index++; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (char === "/" && next === "*") { | ||
| index += 2; | ||
| while (index < source.length && !(source[index] === "*" && source[index + 1] === "/")) { | ||
| index++; | ||
| } | ||
| index = Math.min(index + 2, source.length); | ||
| continue; | ||
| } | ||
|
|
||
| return index; | ||
| } | ||
|
|
||
| return index; | ||
| } | ||
|
|
||
| function readDirectiveLiteral( | ||
| source: string, | ||
| start: number, | ||
| ): { value: string; end: number } | null { | ||
| const quote = source[start]; | ||
| if (quote !== '"' && quote !== "'") { | ||
| return null; | ||
| } | ||
|
|
||
| let value = ""; | ||
| let index = start + 1; | ||
| while (index < source.length) { | ||
| const char = source[index]; | ||
| if (char === quote) { | ||
| return { value, end: index + 1 }; | ||
| } | ||
| if (char === "\\") { | ||
| const escaped = source[index + 1]; | ||
| if (escaped === undefined) { | ||
| return null; | ||
| } | ||
| // This scanner only needs directive equality, not full JavaScript string semantics. | ||
| value += escaped; | ||
|
Comment on lines
+104
to
+110
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The escape handling here treats Not a correctness issue for the |
||
| index += 2; | ||
| continue; | ||
| } | ||
| value += char; | ||
| index++; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| export function hasModuleUseServerDirective(source: string): boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this function has a subtle correctness assumption worth documenting. The directive prologue loop correctly handles:
It correctly rejects:
The bare-semicolon case is interesting: No change needed, just confirming the behavior is correct. |
||
| let index = source.charCodeAt(0) === 0xfeff ? 1 : 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The BOM skip is good, but the parser doesn't handle hashbangs ( In practice nobody writes |
||
|
|
||
| while (index < source.length) { | ||
| index = skipWhitespaceAndComments(source, index); | ||
| const directive = readDirectiveLiteral(source, index); | ||
| if (!directive) { | ||
| return false; | ||
| } | ||
| if (directive.value === "use server") { | ||
| return true; | ||
| } | ||
| index = skipWhitespaceAndComments(source, directive.end); | ||
| if (source[index] === ";") { | ||
| index++; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export async function collectServerActionWarmupEntries( | ||
| options: CollectServerActionWarmupEntriesOptions, | ||
| ): Promise<string[]> { | ||
| const extensions = normalizeServerActionExtensions(options.pageExtensions); | ||
| const pattern = `**/*.${buildExtensionGlob(extensions)}`; | ||
| const entries: string[] = []; | ||
|
|
||
| for await (const relativeFile of glob(pattern, { | ||
| cwd: options.root, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The scan runs from The test at line 121 confirms this is intentional (it expects Two things to consider:
Not blocking — the broader scan is arguably more correct since server actions can live anywhere — but the description should match the implementation. |
||
| exclude: shouldExcludeServerActionScanPath, | ||
| })) { | ||
| const filePath = path.join(options.root, relativeFile); | ||
| const source = await readFile(filePath, "utf8"); | ||
| if (hasModuleUseServerDirective(source)) { | ||
| entries.push(toViteEntry(options.root, filePath)); | ||
| } | ||
| } | ||
|
|
||
| return entries.sort(); | ||
| } | ||
|
|
||
| export function mergeServerActionWarmupEntries( | ||
| userWarmup: readonly string[] | undefined, | ||
| actionWarmup: readonly string[], | ||
| ): string[] { | ||
| return [...new Set([...(userWarmup ?? []), ...actionWarmup])]; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The segment-based exclusion works, but note that
segments[0]checks the first path component relative tocwd(which isroot). This means directories like.git,dist,build,coverage,outare excluded only when they're direct children of root.A nested
build/ordist/directory (e.g.,packages/foo/dist/actions.tsin a monorepo) would NOT be excluded. Thenode_modulescheck covers the most important case since it checkssegments.includes("node_modules"), but the other excluded names only match at the root level.This is probably fine for the initial implementation — most projects have these directories at root level. Just flagging the asymmetry between the
node_modulescheck (any depth) and the other exclusions (root only).