-
Notifications
You must be signed in to change notification settings - Fork 0
perf(services): keep the services snapshot out of the client route chunk #890
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
Merged
+198
−29
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
ac8434f
perf(services): keep the services snapshot out of the client route chunk
claude ceabc04
Merge branch 'main' into claude/clinical-kb-pwa-review-asi3wb
BigSimmo de587a2
docs: record the Phase 4 reviewed HEAD SHA in its ledger row
claude 92a92ea
Merge branch 'main' into claude/clinical-kb-pwa-review-asi3wb
BigSimmo 57b2802
test: lock the services snapshot out of client modules
claude b1f3c90
test: cover transitive graphs and refine import classification in the…
claude d955b58
Merge branch 'main' into claude/clinical-kb-pwa-review-asi3wb
BigSimmo 83c9293
test: match resolved paths for the services boundary guard
claude 480291c
Merge branch 'main' into claude/clinical-kb-pwa-review-asi3wb
BigSimmo 8386245
test: detect dynamic imports and full directive prologues in the serv…
claude 0e74e84
test: match magic-comment dynamic imports in the services guard
claude 4f981b7
Merge branch 'main' into claude/clinical-kb-pwa-review-asi3wb
BigSimmo 3253357
Merge branch 'main' into claude/clinical-kb-pwa-review-asi3wb
BigSimmo 649a325
docs: keep the Phase 4 reviewed-HEAD ledger cell to the bare SHA
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| import { existsSync, readFileSync, readdirSync, statSync } from "node:fs"; | ||
| import { dirname, join, relative, resolve } from "node:path"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| // Regression guard for the PR #890 fix: the ~915 KB services snapshot reaches | ||
| // the client the moment "@/lib/services" is value-imported anywhere in a | ||
| // client module graph (~100 KB gzip in that chunk). The enforced bundle | ||
| // budget alone cannot catch a re-introduction — its 10% tolerance sits above | ||
| // the pre-fix total — so the boundary is asserted at the source level, and it | ||
| // must be TRANSITIVE: a helper without its own "use client" directive still | ||
| // lands in the client bundle when a client component imports it. Type-only | ||
| // imports are erased at compile time and stay allowed. | ||
| const SRC_ROOT = join(process.cwd(), "src"); | ||
| const TARGET_SPECIFIER = "@/lib/services"; | ||
| // Side-effect imports have no `from` clause but still execute the module. | ||
| const SIDE_EFFECT_IMPORT_PATTERN = /^import\s+["']([^"']+)["']/gm; | ||
| // Dynamic import() expressions defer loading but still emit client JavaScript | ||
| // for the target, so they count as runtime edges too — including with | ||
| // webpack/Next magic comments before the specifier. | ||
| const DYNAMIC_IMPORT_PATTERN = /\bimport\s*\(\s*(?:\/\*[\s\S]*?\*\/\s*)*["']([^"']+)["'][\s\S]*?\)/g; | ||
| // import/export ... from "..." — clause analysed by hasRuntimeBindings below so | ||
| // `import type`, `export type`, and named clauses whose specifiers are all | ||
| // `type X` (including multiline) stay allowed while default, namespace, mixed, | ||
| // star re-export, and value re-export forms are treated as runtime. | ||
| const FROM_STATEMENT_PATTERN = /^(import|export)\s+([\s\S]+?)\s+from\s+["']([^"']+)["']/gm; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| function hasRuntimeBindings(kind: string, clause: string): boolean { | ||
| const trimmed = clause.trim(); | ||
| if (/^type\b/.test(trimmed)) return false; | ||
| const named = trimmed.match(/^\{([\s\S]*)\}$/); | ||
| if (named) { | ||
| return named[1] | ||
| .split(",") | ||
| .map((specifier) => specifier.trim()) | ||
| .filter(Boolean) | ||
| .some((specifier) => !/^type\b/.test(specifier)); | ||
| } | ||
| return kind === "import" || kind === "export"; | ||
| } | ||
|
|
||
| function collectSourceFiles(dir: string): string[] { | ||
| return readdirSync(dir).flatMap((entry) => { | ||
| const fullPath = join(dir, entry); | ||
| if (statSync(fullPath).isDirectory()) return collectSourceFiles(fullPath); | ||
| return /\.(?:ts|tsx)$/.test(entry) ? [fullPath] : []; | ||
| }); | ||
| } | ||
|
|
||
| function resolveImport(specifier: string, fromFile: string): string | null { | ||
| let base: string; | ||
| if (specifier.startsWith("@/")) base = join(SRC_ROOT, specifier.slice(2)); | ||
| else if (specifier.startsWith(".")) base = resolve(dirname(fromFile), specifier); | ||
| else return null; | ||
|
|
||
| for (const candidate of [base, `${base}.ts`, `${base}.tsx`, join(base, "index.ts"), join(base, "index.tsx")]) { | ||
| if (existsSync(candidate) && statSync(candidate).isFile()) return candidate; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| // Every on-disk shape the "@/lib/services" module could resolve to, so | ||
| // relative-path imports are treated identically to the alias. | ||
| const SERVICES_MODULE_PATHS = new Set( | ||
| ["lib/services.ts", "lib/services.tsx", "lib/services/index.ts", "lib/services/index.tsx"].map((candidate) => | ||
| join(SRC_ROOT, candidate), | ||
| ), | ||
| ); | ||
|
|
||
| // Strip the full directive prologue (comments and whitespace of any length) | ||
| // so a "use client" directive after a long header comment is still detected. | ||
| function isClientEntry(source: string): boolean { | ||
| const prologue = source.replace(/^(?:\s+|\/\/[^\n]*(?:\n|$)|\/\*[\s\S]*?\*\/)*/, ""); | ||
| return /^["']use client["']/.test(prologue); | ||
| } | ||
|
|
||
| interface ModuleInfo { | ||
| importsServices: boolean; | ||
| isClientEntry: boolean; | ||
| valueImports: string[]; | ||
| } | ||
|
|
||
| function buildModuleGraph(): Map<string, ModuleInfo> { | ||
| const graph = new Map<string, ModuleInfo>(); | ||
|
|
||
| for (const filePath of collectSourceFiles(SRC_ROOT)) { | ||
| const source = readFileSync(filePath, "utf8"); | ||
| const valueImports: string[] = []; | ||
| let importsServices = false; | ||
|
|
||
| const recordRuntimeSpecifier = (specifier: string) => { | ||
| const resolved = resolveImport(specifier, filePath); | ||
| // Match the alias text AND the resolved file, so relative specifiers | ||
| // like ../lib/services are caught identically. | ||
| if ( | ||
| specifier === TARGET_SPECIFIER || | ||
| specifier.startsWith(`${TARGET_SPECIFIER}/`) || | ||
| (resolved !== null && SERVICES_MODULE_PATHS.has(resolved)) | ||
| ) { | ||
| importsServices = true; | ||
| } | ||
| if (resolved) valueImports.push(resolved); | ||
| }; | ||
|
|
||
| for (const match of source.matchAll(SIDE_EFFECT_IMPORT_PATTERN)) recordRuntimeSpecifier(match[1]); | ||
| for (const match of source.matchAll(DYNAMIC_IMPORT_PATTERN)) recordRuntimeSpecifier(match[1]); | ||
| for (const match of source.matchAll(FROM_STATEMENT_PATTERN)) { | ||
| if (hasRuntimeBindings(match[1], match[2])) recordRuntimeSpecifier(match[3]); | ||
|
BigSimmo marked this conversation as resolved.
|
||
| } | ||
|
|
||
| graph.set(filePath, { | ||
| importsServices, | ||
| isClientEntry: isClientEntry(source), | ||
| valueImports, | ||
|
BigSimmo marked this conversation as resolved.
|
||
| }); | ||
| } | ||
|
|
||
| return graph; | ||
| } | ||
|
|
||
| describe("services snapshot client boundary", () => { | ||
| it("keeps @/lib/services value-imports out of every client module graph", () => { | ||
| const graph = buildModuleGraph(); | ||
| const offenders: string[] = []; | ||
|
|
||
| for (const [entryPath, entry] of graph) { | ||
| if (!entry.isClientEntry) continue; | ||
|
|
||
| const cameFrom = new Map<string, string>([[entryPath, ""]]); | ||
| const queue = [entryPath]; | ||
| while (queue.length > 0) { | ||
| const currentPath = queue.shift() as string; | ||
| const current = graph.get(currentPath); | ||
| if (!current) continue; | ||
|
|
||
| if (current.importsServices) { | ||
| const chain: string[] = []; | ||
| for (let step: string | undefined = currentPath; step; step = cameFrom.get(step) || undefined) { | ||
| chain.unshift(relative(process.cwd(), step)); | ||
| } | ||
| offenders.push(chain.join(" -> ")); | ||
| break; | ||
| } | ||
| for (const next of current.valueImports) { | ||
| if (!cameFrom.has(next)) { | ||
| cameFrom.set(next, currentPath); | ||
| queue.push(next); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| expect( | ||
| offenders, | ||
| "Client module graphs must not value-import @/lib/services: it compiles the full services " + | ||
| "snapshot into their chunk. Compute what you need server-side and pass it as a prop " + | ||
| "(see src/app/services/page.tsx), or use `import type` for types only. Chains shown as " + | ||
| "client entry -> ... -> importing module.", | ||
| ).toEqual([]); | ||
| }); | ||
| }); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.