From 54f1ed653986f6580f18813b73ee0511f4a929f8 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 19:59:24 -0700 Subject: [PATCH 01/23] test: guard against server components reading client-module values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every export of a `'use client'` module is a client reference on the server, not the value it looks like. A Server Component that reads one gets an opaque placeholder — React writes it into the flight payload as `"$56"` — and the real value only appears once the client resolves the reference during hydration. Nothing fails. That is what makes it worth a test. A profile branch had the space layout write `id={SPACE_TABS_ANCHOR}` with the constant living in a client module, and the deployed HTML came back carrying `{"id":"$56"}`: an element with no id until hydration, and a fragment link with nothing to scroll to. It looked right in the browser and every test passed. This walks the server render graph from every layout, page, loading state and route handler, and fails on any non-component value taken from a client module — named, default, namespace or re-exported. Type-only edges are excluded, which is load-bearing rather than tidy: the sole route into `core/blocks/data/filters.ts` is an `import type`, and counting it walks into the sync store and reports three modules TypeScript erases before anything runs. Five cases exist today. They are listed with what each one does and what fixing it would take, rather than fixed here: one is live (`app/bounties/loading.tsx` renders `className={BOARD_GRID_CLASS}` from a client module), one would throw, one is a landmine, one is inert. The list is checked for staleness too, so an entry that gets fixed cannot sit here re-permitting the same import later. Sibling of `async-components-in-client-boundaries.test.ts`, which guards the other direction of the same boundary. --- ...client-values-in-server-boundaries.test.ts | 268 ++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts new file mode 100644 index 0000000000..933391005d --- /dev/null +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -0,0 +1,268 @@ +// This walks the source tree with `fs` and never touches the DOM. +// @vitest-environment node +import { existsSync, readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** + * Every export of a `'use client'` module is a *client reference* on the server, not the value it + * looks like. A Server Component that reads one gets an opaque placeholder: React writes it into + * the flight payload as `"$56"`, and the real value only appears once the client resolves the + * reference during hydration. + * + * Nothing fails. That is the whole problem. A profile branch had the space layout write + * `id={SPACE_TABS_ANCHOR}` with the constant living in `space-tabs.tsx`, and the deployed server + * HTML came back as `["$","div",null,{"id":"$56","className":"scroll-mt-14"}]` — an element with + * no id until hydration, and a fragment link with nothing to scroll to. In the browser it looked + * perfect, and every test passed (GEO-2974). + * + * The sibling of this test guards the other direction — async components rendered from client + * files. Same failure mode: correct-looking UI, wrong boundary. + * + * What this cannot see: whether the value is ever *read* while rendering on the server. A client + * hook imported next to a server-safe constant and only ever called from a client component is + * inert. So the allowlist below is not a list of things that are fine — it is a list of things + * checked by hand, each with what was found. + * + * Nor does it follow `import()` or a bare `import './x'`. Neither reaches a source module from the + * server graph today — checked, not assumed: the 85 files calling `import()` are client modules + * reaching for `next/dynamic`, and every bare import in the graph resolves to CSS or a package. + * Worth adding the day either stops being true. + */ + +const ROOT = path.resolve(__dirname, '..', '..'); +const SOURCE_DIRS = ['app', 'core', 'partials', 'design-system']; + +/** The files Next renders on the server by definition. Everything they reach is the server graph. */ +const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; + +/** + * Every way one module reaches another *at runtime*, because a traversal that follows only one of + * them walks a smaller graph than the server actually renders and quietly stops guarding the rest + * of it. The tree uses all of these: ~9800 named imports, ~340 default, ~870 namespace, 53 + * re-exports. + * + * `import type` is excluded, and it matters: the only route into `core/blocks/data/filters.ts` is a + * type import from `core/chat/edit-types.ts`, so counting it walks into the sync store and reports + * three modules that TypeScript erases before anything runs. + */ +const MODULE_EDGE = /^(?:import|export)\s+(?!type\s)[\s\S]*?from\s+['"]([^'"]+)['"]/gm; + +/** `import { a, b as c } from '…'`, with or without a default binding in front. */ +const NAMED_IMPORT_BLOCK = /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; + +/** `import Local from '…'`, ignoring the `import type` and `import * as` forms. */ +const DEFAULT_IMPORT = /^import\s+(?!type\s)([A-Za-z_$][\w$]*)\s*(?:,\s*\{[^}]*\})?\s+from\s+['"]([^'"]+)['"]/gm; + +/** `import * as Local from '…'`, where every property read is a client reference. */ +const NAMESPACE_IMPORT = /^import\s+\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+['"]([^'"]+)['"]/gm; + +/** `export { a } from '…'` and `export * from '…'`, which hand a client reference straight on. */ +const NAMED_REEXPORT = /^export\s+(?!type\s)\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; + +/** + * `export * from '…'` and `export * as Name from '…'`. + * + * The named form is the common one here — 11 files against 4 — so a matcher that only knew the + * bare `export *` was blind to most of the barrels in the tree. + */ +const STAR_REEXPORT = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"]([^'"]+)['"]/gm; + +/** + * What the tree holds today, each one read before being listed rather than swept up by the walk. + * + * This list is not "these are fine". It is the debt this guard found on the day it was written, + * ordered by how much it matters, and the fix for every one of them is the same shape: move the + * value into a module with no `'use client'` on it and import it from both sides. + * + * - `bounty-board-skeleton` is the live one. `app/bounties/loading.tsx` is server-rendered and puts + * `BOARD_GRID_CLASS` straight into a `className`, so the grid has no grid during the loading + * flash. Left here rather than fixed because `BOARD_CARD_HEIGHT_PX` derives from + * `AVAILABLE_CARD_HEIGHT_PX` in a second client module, so the fix relocates layout constants + * across two features and wants someone who can look at the bounties board while doing it. + * - `read-block-media-dimensions` would return a client reference in place of its empty-dimensions + * object. Nothing calls it outside its own test today, so it is a landmine rather than a fault. + * - `entity-response` calls `getChecked` while deriving a response kind. A server caller would + * throw rather than render something wrong, which is the better failure of the two. + * - `bounties/config` is inert: `useFeatureFlag` is only ever called from `useBountiesEnabled`, + * which is a client hook. The module is in the server graph for `bountiesEnabledForNetwork`. + * Untangling it moves a hook out of `config.ts` and repoints nine files, for no behaviour change. + */ +const KNOWN = new Set([ + 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_CARD_HEIGHT_PX', + 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_GRID_CLASS', + 'core/blocks/data/read-block-media-dimensions.ts -> NO_BLOCK_MEDIA_DIMENSIONS', + 'core/responses/entity-response.ts -> getChecked', + 'core/bounties/config.ts -> useFeatureFlag', +]); + +function sourceFiles(): string[] { + const found: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(path.join(ROOT, dir), { withFileTypes: true })) { + const rel = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name !== 'node_modules') walk(rel); + } else if (/\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name)) { + found.push(rel); + } + } + }; + for (const dir of SOURCE_DIRS) walk(dir); + return found; +} + +function isClientFile(contents: string): boolean { + return /^\s*['"]use client['"]/.test(contents); +} + +function resolveImport(specifier: string, importingFile: string): string | null { + let absolute: string; + + if (specifier.startsWith('~/')) { + absolute = path.join(ROOT, specifier.slice(2)); + } else if (specifier.startsWith('.')) { + absolute = path.resolve(ROOT, path.dirname(importingFile), specifier); + } else { + return null; + } + + const relative = path.relative(ROOT, absolute); + for (const candidate of [ + `${relative}.tsx`, + `${relative}.ts`, + path.join(relative, 'index.tsx'), + path.join(relative, 'index.ts'), + ]) { + if (!SOURCE_DIRS.some(dir => candidate.startsWith(`${dir}${path.sep}`))) continue; + if (existsSync(path.join(ROOT, candidate))) return candidate; + } + return null; +} + +/** + * `A` or `A as B` inside a named import block, skipping inline `type` specifiers. + * + * The exported name is what identifies the export, so `foo as bar` is judged as `foo`. `default as + * Bar` is the exception: `default` says nothing about what it is, so the local name is the only + * signal there — the same reasoning as a default import. + */ +function parseNamedBindings(block: string): string[] { + return block + .split(',') + .map(entry => entry.trim()) + .filter(entry => entry.length > 0 && !entry.startsWith('type ')) + .map(entry => { + const [exported, local] = entry.split(/\s+as\s+/).map(part => part.trim()); + return exported === 'default' && local ? local : exported; + }) + .filter(Boolean); +} + +/** + * Components are the one export a Server Component may take from a client module — that is what the + * boundary is for. PascalCase stands in for "component", with SCREAMING_CASE excluded, since + * `BOARD_GRID_CLASS` passes a naive capital-letter test while being a string. + */ +function looksLikeComponent(name: string): boolean { + return /^[A-Z]/.test(name) && name !== name.toUpperCase(); +} + +describe('server components take only components from client modules', () => { + const files = sourceFiles(); + const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); + const clientFiles = new Set([...contentsByFile].filter(([, c]) => isClientFile(c)).map(([f]) => f)); + + it('walks a source tree that actually has files in it', () => { + // Guards against a silently empty run if the layout moves. + expect(files.length).toBeGreaterThan(50); + expect(clientFiles.size).toBeGreaterThan(50); + }); + + /** Every non-client module reachable from a server entry point, which is where this can bite. */ + const serverGraph = new Set(); + const queue = files.filter(file => SERVER_ENTRY.test(file.split(path.sep).join('/')) && !clientFiles.has(file)); + + // The seeds are the layouts, pages and loading states. If this is empty the walk above drifted. + expect(queue.length).toBeGreaterThan(20); + + while (queue.length > 0) { + const file = queue.pop()!; + if (serverGraph.has(file) || clientFiles.has(file)) continue; + serverGraph.add(file); + + for (const match of (contentsByFile.get(file) ?? '').matchAll(MODULE_EDGE)) { + const target = resolveImport(match[1], file); + if (target && !clientFiles.has(target) && !serverGraph.has(target)) queue.push(target); + } + } + + it('reaches a server graph worth checking', () => { + expect(serverGraph.size).toBeGreaterThan(100); + }); + + /** + * Every value a server-graph module takes from a client module, in any of the shapes it can + * arrive in, as `[offence, source]` pairs. A default binding is judged by its local name — the + * only signal a default import carries — and a namespace binding is reported whole, since every + * property read off it is a client reference. + */ + function* clientValuesInServerGraph(): Generator<[string, string]> { + for (const file of serverGraph) { + const contents = contentsByFile.get(file)!; + const from = file.split(path.sep).join('/'); + + const fromClientModule = (specifier: string) => { + const target = resolveImport(specifier, file); + return target && clientFiles.has(target) ? target.split(path.sep).join('/') : null; + }; + + for (const [, block, specifier] of contents.matchAll(NAMED_IMPORT_BLOCK)) { + const target = fromClientModule(specifier); + if (!target) continue; + for (const name of parseNamedBindings(block)) { + if (!looksLikeComponent(name)) yield [`${from} -> ${name}`, target]; + } + } + + for (const [, local, specifier] of contents.matchAll(DEFAULT_IMPORT)) { + const target = fromClientModule(specifier); + if (target && !looksLikeComponent(local)) yield [`${from} -> default as ${local}`, target]; + } + + for (const [, local, specifier] of contents.matchAll(NAMESPACE_IMPORT)) { + const target = fromClientModule(specifier); + if (target) yield [`${from} -> * as ${local}`, target]; + } + + for (const [, block, specifier] of contents.matchAll(NAMED_REEXPORT)) { + const target = fromClientModule(specifier); + if (!target) continue; + for (const name of parseNamedBindings(block)) { + if (!looksLikeComponent(name)) yield [`${from} -> re-exports ${name}`, target]; + } + } + + for (const [, namespace, specifier] of contents.matchAll(STAR_REEXPORT)) { + const target = fromClientModule(specifier); + if (target) yield [`${from} -> re-exports ${namespace ? `* as ${namespace}` : '*'}`, target]; + } + } + } + + it('finds no non-component value taken from a "use client" module', () => { + const offences = [...clientValuesInServerGraph()] + .filter(([offence]) => !KNOWN.has(offence)) + .map(([offence, target]) => `${offence} (from ${target})`); + + expect(offences).toEqual([]); + }); + + it('keeps the known list honest', () => { + // An entry that no longer matches anything has been fixed, and leaving it here would quietly + // re-permit the same import later. + const live = new Set([...clientValuesInServerGraph()].map(([offence]) => offence)); + + expect([...KNOWN].filter(entry => !live.has(entry))).toEqual([]); + }); +}); From 50d67379d7f595fa84d4bb6fb33479cc23158847 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 12:20:59 -0700 Subject: [PATCH 02/23] test: judge a client-module export by how it is used, not how it is spelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the hole: capitalisation alone let `const DefaultConfig = {...}` through, and a client module exporting a capitalised value is exactly the case this test exists for. Planted one and it sailed past. Three questions now, cheapest first: is it capitalised, is it used here as a component — rendered as `` or handed on as `render={Name}` — and does the source declare it as a type the compiler erases. Capitalised and neither of the last two is a value wearing a component's name. Measured against the tree rather than reasoned about: of 163 capitalised imports from client modules in the server graph, 161 are rendered as JSX where they are imported, and the other two are `export type` imported without the `type` keyword — `Tabs` from `editor-provider` and `Feature` from `use-place-search`. So the type check is not defensive, it is the difference between this landing clean and reporting two files that do nothing at runtime. The prop form is for a case the tree does not hold yet — a server module taking a client component and passing it on without rendering it — because the failure would otherwise be a false accusation, and a guard that cries wolf gets deleted. Proved all three by planting them: the value is caught, the rendered component and the passed-on component are not. Doc block now says what is still uncovered, which the review asked for: a component neither rendered nor passed on here would be reported, and a capitalised value re-exported from a barrel is let through, since a re-export has no use site to read. --- ...client-values-in-server-boundaries.test.ts | 92 +++++++++++++++---- 1 file changed, 76 insertions(+), 16 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 933391005d..aa41391a6a 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -28,6 +28,14 @@ import { describe, expect, it } from 'vitest'; * server graph today — checked, not assumed: the 85 files calling `import()` are client modules * reaching for `next/dynamic`, and every bare import in the graph resolves to CSS or a package. * Worth adding the day either stops being true. + * + * And it decides what counts as a component by reading the source, not by resolving types — see + * `isComponentHere`. It is right about the 163 capitalised imports in the tree today, and the two + * places it could still be wrong are worth knowing: a component this file imports and neither + * renders nor passes on would be reported, and a capitalised value *re-exported* from a barrel is + * let through, because a re-export has no use site to read. Both are conservative in the direction + * of the failure being visible rather than silent, which is the only direction that works for a + * test nobody runs deliberately. */ const ROOT = path.resolve(__dirname, '..', '..'); @@ -143,31 +151,57 @@ function resolveImport(specifier: string, importingFile: string): string | null /** * `A` or `A as B` inside a named import block, skipping inline `type` specifiers. * - * The exported name is what identifies the export, so `foo as bar` is judged as `foo`. `default as - * Bar` is the exception: `default` says nothing about what it is, so the local name is the only - * signal there — the same reasoning as a default import. + * Both names are kept because they answer different questions. The export is known by its exported + * name, which is what the source module declares; it is used under its local one, which is what + * appears in JSX here. */ -function parseNamedBindings(block: string): string[] { +function parseNamedBindings(block: string): { exported: string; local: string }[] { return block .split(',') .map(entry => entry.trim()) .filter(entry => entry.length > 0 && !entry.startsWith('type ')) .map(entry => { const [exported, local] = entry.split(/\s+as\s+/).map(part => part.trim()); - return exported === 'default' && local ? local : exported; + return { exported, local: local ?? exported }; }) - .filter(Boolean); + .filter(({ exported, local }) => Boolean(exported) && Boolean(local)); } /** * Components are the one export a Server Component may take from a client module — that is what the - * boundary is for. PascalCase stands in for "component", with SCREAMING_CASE excluded, since - * `BOARD_GRID_CLASS` passes a naive capital-letter test while being a string. + * boundary is for. So the question for every binding is whether it is one, and capitalisation alone + * cannot answer it: `BOARD_GRID_CLASS` fails a naive capital-letter test while being a string, and + * a client module exporting `const DefaultConfig = {...}` passes one while being an object. + * + * Three questions instead, cheapest first. */ -function looksLikeComponent(name: string): boolean { +function isCapitalised(name: string): boolean { return /^[A-Z]/.test(name) && name !== name.toUpperCase(); } +/** + * Whether the importing file treats it as a component: rendered as ``, or handed to something + * else to render as `render={Name}`. The sibling test matches render sites the same way. + * + * The prop form is here for a case the tree does not hold yet — a server module importing a client + * component and passing it on without rendering it — because the failure would otherwise be a + * false accusation, and a guard that cries wolf gets deleted. + */ +function usedAsComponent(local: string, contents: string): boolean { + return new RegExp(`<${local}[\\s/>]|=\\{\\s*${local}\\s*\\}`).test(contents); +} + +/** + * Whether the source module declares it as a type, which the compiler erases before anything runs. + * + * Two of these exist today and both would otherwise be reported: `Tabs` from `editor-provider` and + * `Feature` from `use-place-search` are `export type`, imported without the `type` keyword and used + * only in annotations. + */ +function declaredAsType(exported: string, sourceContents: string): boolean { + return new RegExp(`^export\\s+(?:type|interface)\\s+${exported}\\b`, 'm').test(sourceContents); +} + describe('server components take only components from client modules', () => { const files = sourceFiles(); const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); @@ -201,10 +235,31 @@ describe('server components take only components from client modules', () => { expect(serverGraph.size).toBeGreaterThan(100); }); + /** + * Whether this binding is a component as far as this file is concerned — the one thing a server + * module may take across the boundary. Capitalised *and* either used as one here or erased by the + * compiler; capitalised and neither is the `const DefaultConfig = {...}` case, which is a value + * wearing a component's name. + */ + function isComponentHere({ + exported, + local, + contents, + target, + }: { + exported: string; + local: string; + contents: string; + target: string; + }): boolean { + if (!isCapitalised(exported === 'default' ? local : exported)) return false; + + return usedAsComponent(local, contents) || declaredAsType(exported, contentsByFile.get(target) ?? ''); + } + /** * Every value a server-graph module takes from a client module, in any of the shapes it can - * arrive in, as `[offence, source]` pairs. A default binding is judged by its local name — the - * only signal a default import carries — and a namespace binding is reported whole, since every + * arrive in, as `[offence, source]` pairs. A namespace binding is reported whole, since every * property read off it is a client reference. */ function* clientValuesInServerGraph(): Generator<[string, string]> { @@ -220,14 +275,17 @@ describe('server components take only components from client modules', () => { for (const [, block, specifier] of contents.matchAll(NAMED_IMPORT_BLOCK)) { const target = fromClientModule(specifier); if (!target) continue; - for (const name of parseNamedBindings(block)) { - if (!looksLikeComponent(name)) yield [`${from} -> ${name}`, target]; + for (const { exported, local } of parseNamedBindings(block)) { + if (isComponentHere({ exported, local, contents, target })) continue; + yield [`${from} -> ${exported === 'default' ? `default as ${local}` : exported}`, target]; } } for (const [, local, specifier] of contents.matchAll(DEFAULT_IMPORT)) { const target = fromClientModule(specifier); - if (target && !looksLikeComponent(local)) yield [`${from} -> default as ${local}`, target]; + if (!target) continue; + if (isComponentHere({ exported: 'default', local, contents, target })) continue; + yield [`${from} -> default as ${local}`, target]; } for (const [, local, specifier] of contents.matchAll(NAMESPACE_IMPORT)) { @@ -238,8 +296,10 @@ describe('server components take only components from client modules', () => { for (const [, block, specifier] of contents.matchAll(NAMED_REEXPORT)) { const target = fromClientModule(specifier); if (!target) continue; - for (const name of parseNamedBindings(block)) { - if (!looksLikeComponent(name)) yield [`${from} -> re-exports ${name}`, target]; + for (const { exported } of parseNamedBindings(block)) { + // No JSX to look at in a re-export, so capitalisation and the type check are all there is. + if (isCapitalised(exported) || declaredAsType(exported, contentsByFile.get(target) ?? '')) continue; + yield [`${from} -> re-exports ${exported}`, target]; } } From 31be2b17fab39dcc5240858ba90583174d41f8ef Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 12:54:25 -0700 Subject: [PATCH 03/23] test: parse declarations instead of pattern-matching near them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings, all of them real, and six were the same mistake in different places: a regex standing in where parsing was needed. `[\s\S]*?` does not stop at the end of a declaration. In `app/layout.tsx` the one loose matcher started on a bare `import 'katex/…css';`, walked past two statements and captured a later specifier — three overruns in one small file, and every statement it swallowed was skipped. It would equally start on an `export const` and run until it found a `from` dozens of lines away. There is a matcher per declaration shape now, each bounded to its own, and the traversal and the detection read from the same set. That set also grew. `import Default, * as Namespace from './client'` matched neither of the old two patterns and both bindings went through. `import()` and bare imports are followed rather than assumed harmless — the assumption held when I measured it, but nothing kept it true, and a module reachable only through `await import()` was unguarded. `atoms` was missing from the roots entirely, which fails twice over: never loaded, and every `~/atoms/…` specifier resolving to null. Classification moves to the declaration. Asking the use site let `config={DefaultConfig}` prove an object was a component, and had nothing at all to say about a re-export. Reading how the client module declares the export answers both: 144 of the tree's capitalised imports are `export function`, 17 an arrow or memo/forwardRef, 2 an `export type` imported without the keyword, and nothing is unclassifiable — so `unknown` is reported rather than waved through. Re-exports keep their alias, so `export { default as Button }` stops being reported and `export { DefaultConfig }` starts. The allowlist is keyed by client module as well as importer and name. Without it an entry went on authorising the same name after the import was repointed at a different client module — debt licensing a new fault. Every one of the seven was planted before and after: caught by the new version, missed by the old, and the two that should stay silent do. The per-root assertion earned its place immediately by catching `styles` being added to the roots when it holds nothing but a test. Server graph: 377 modules two rounds ago, 402 last round, 407 now. --- ...client-values-in-server-boundaries.test.ts | 304 ++++++++++-------- 1 file changed, 170 insertions(+), 134 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index aa41391a6a..1f90f4ac4e 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -19,62 +19,76 @@ import { describe, expect, it } from 'vitest'; * The sibling of this test guards the other direction — async components rendered from client * files. Same failure mode: correct-looking UI, wrong boundary. * - * What this cannot see: whether the value is ever *read* while rendering on the server. A client - * hook imported next to a server-safe constant and only ever called from a client component is - * inert. So the allowlist below is not a list of things that are fine — it is a list of things - * checked by hand, each with what was found. + * Two things it deliberately cannot see, both of which make the allowlist a list of things checked + * by hand rather than a list of things that are fine: * - * Nor does it follow `import()` or a bare `import './x'`. Neither reaches a source module from the - * server graph today — checked, not assumed: the 85 files calling `import()` are client modules - * reaching for `next/dynamic`, and every bare import in the graph resolves to CSS or a package. - * Worth adding the day either stops being true. - * - * And it decides what counts as a component by reading the source, not by resolving types — see - * `isComponentHere`. It is right about the 163 capitalised imports in the tree today, and the two - * places it could still be wrong are worth knowing: a component this file imports and neither - * renders nor passes on would be reported, and a capitalised value *re-exported* from a barrel is - * let through, because a re-export has no use site to read. Both are conservative in the direction - * of the failure being visible rather than silent, which is the only direction that works for a - * test nobody runs deliberately. + * 1. Whether the value is ever *read* while rendering on the server. A client hook sitting next to + * a server-safe constant and only ever called from a client component is inert. + * 2. What a dynamically imported module's bindings are. The edge is followed, so everything beyond + * it is still guarded, but `const { x } = await import('./client')` is not destructured here. + * No server-graph module does that today. */ const ROOT = path.resolve(__dirname, '..', '..'); -const SOURCE_DIRS = ['app', 'core', 'partials', 'design-system']; - -/** The files Next renders on the server by definition. Everything they reach is the server graph. */ -const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; /** - * Every way one module reaches another *at runtime*, because a traversal that follows only one of - * them walks a smaller graph than the server actually renders and quietly stops guarding the rest - * of it. The tree uses all of these: ~9800 named imports, ~340 default, ~870 namespace, 53 - * re-exports. + * Every top-level directory holding application source. + * + * `atoms` was missing — six jotai modules — and a missing root fails twice over: the walk never + * loads the files, and `resolveImport` answers null for every `~/atoms/…` specifier, so an import + * from there was invisible rather than merely unchecked. * - * `import type` is excluded, and it matters: the only route into `core/blocks/data/filters.ts` is a - * type import from `core/chat/edit-types.ts`, so counting it walks into the sync store and reports - * three modules that TypeScript erases before anything runs. + * Two are left out on purpose, both checked rather than assumed: `scripts` is build tooling that + * nothing under `app/` imports, and `styles` holds one test file and its CSS. The per-root + * assertion below is what caught `styles` being added here by mistake. */ -const MODULE_EDGE = /^(?:import|export)\s+(?!type\s)[\s\S]*?from\s+['"]([^'"]+)['"]/gm; - -/** `import { a, b as c } from '…'`, with or without a default binding in front. */ -const NAMED_IMPORT_BLOCK = /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; +const SOURCE_DIRS = ['app', 'atoms', 'core', 'design-system', 'partials']; -/** `import Local from '…'`, ignoring the `import type` and `import * as` forms. */ -const DEFAULT_IMPORT = /^import\s+(?!type\s)([A-Za-z_$][\w$]*)\s*(?:,\s*\{[^}]*\})?\s+from\s+['"]([^'"]+)['"]/gm; - -/** `import * as Local from '…'`, where every property read is a client reference. */ -const NAMESPACE_IMPORT = /^import\s+\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+['"]([^'"]+)['"]/gm; - -/** `export { a } from '…'` and `export * from '…'`, which hand a client reference straight on. */ -const NAMED_REEXPORT = /^export\s+(?!type\s)\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; +/** The files Next renders on the server by definition. Everything they reach is the server graph. */ +const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; /** - * `export * from '…'` and `export * as Name from '…'`. + * One matcher per declaration shape, rather than one loose pattern for all of them. + * + * The loose version was `^(?:import|export)\s+(?!type\s)[\s\S]*?from\s+['"](…)['"]`, and `[\s\S]*?` + * does not stop at the end of a declaration. In `app/layout.tsx` it started on a bare + * `import 'katex/dist/katex.min.css';`, walked past two more statements and captured the specifier + * of a later one — so edges were attributed to declarations that do not have them, and any + * statement in between was skipped because the match had already consumed it. It would equally + * start on an `export const` and run until it found a `from` dozens of lines away. + * + * Each of these is bounded to its own shape: a `{…}` block cannot contain a `}`, and nothing else + * crosses a declaration boundary. Between them they cover what the tree actually uses — ~9800 named + * imports, ~340 default, ~870 namespace, 53 re-exports, 136 bare, 85 dynamic. * - * The named form is the common one here — 11 files against 4 — so a matcher that only knew the - * bare `export *` was blind to most of the barrels in the tree. + * `import type` and `export type` are excluded, and that exclusion is load-bearing: the only route + * into `core/blocks/data/filters.ts` is a type import from `core/chat/edit-types.ts`, and counting + * it walks on into the sync store and reports three modules TypeScript erases before anything runs. */ -const STAR_REEXPORT = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"]([^'"]+)['"]/gm; +const IMPORT_NAMED = /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; +const IMPORT_DEFAULT = + /^import\s+(?!type\s)([A-Za-z_$][\w$]*)\s*(?:,\s*(?:\{[^}]*\}|\*\s+as\s+[A-Za-z_$][\w$]*))?\s+from\s+['"]([^'"]+)['"]/gm; +const IMPORT_NAMESPACE = + /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+['"]([^'"]+)['"]/gm; +const REEXPORT_NAMED = /^export\s+(?!type\s)\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; +const REEXPORT_STAR = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"]([^'"]+)['"]/gm; +const IMPORT_BARE = /^import\s+['"]([^'"]+)['"]/gm; +const IMPORT_DYNAMIC = /\bimport\(\s*['"]([^'"]+)['"]\s*\)/g; + +/** Every specifier a module pulls in at runtime, whatever shape the declaration took. */ +function runtimeSpecifiers(contents: string): string[] { + const found: string[] = []; + + for (const [, , specifier] of contents.matchAll(IMPORT_NAMED)) found.push(specifier); + for (const [, , specifier] of contents.matchAll(IMPORT_DEFAULT)) found.push(specifier); + for (const [, , specifier] of contents.matchAll(IMPORT_NAMESPACE)) found.push(specifier); + for (const [, , specifier] of contents.matchAll(REEXPORT_NAMED)) found.push(specifier); + for (const [, , specifier] of contents.matchAll(REEXPORT_STAR)) found.push(specifier); + for (const [, specifier] of contents.matchAll(IMPORT_BARE)) found.push(specifier); + for (const [, specifier] of contents.matchAll(IMPORT_DYNAMIC)) found.push(specifier); + + return found; +} /** * What the tree holds today, each one read before being listed rather than swept up by the walk. @@ -83,6 +97,10 @@ const STAR_REEXPORT = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"] * ordered by how much it matters, and the fix for every one of them is the same shape: move the * value into a module with no `'use client'` on it and import it from both sides. * + * The client module is part of the key, not decoration. Keyed on importer and name alone, an + * existing entry would go on authorising the same name after someone repointed the import at a + * *different* client module — debt quietly licensing a new fault. + * * - `bounty-board-skeleton` is the live one. `app/bounties/loading.tsx` is server-rendered and puts * `BOARD_GRID_CLASS` straight into a `className`, so the grid has no grid during the loading * flash. Left here rather than fixed because `BOARD_CARD_HEIGHT_PX` derives from @@ -97,11 +115,11 @@ const STAR_REEXPORT = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"] * Untangling it moves a hook out of `config.ts` and repoints nine files, for no behaviour change. */ const KNOWN = new Set([ - 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_CARD_HEIGHT_PX', - 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_GRID_CLASS', - 'core/blocks/data/read-block-media-dimensions.ts -> NO_BLOCK_MEDIA_DIMENSIONS', - 'core/responses/entity-response.ts -> getChecked', - 'core/bounties/config.ts -> useFeatureFlag', + 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_CARD_HEIGHT_PX (from partials/bounties/board-bounty-card.tsx)', + 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_GRID_CLASS (from partials/bounties/board-bounty-card.tsx)', + 'core/blocks/data/read-block-media-dimensions.ts -> NO_BLOCK_MEDIA_DIMENSIONS (from core/hooks/use-block-media-dimensions.ts)', + 'core/responses/entity-response.ts -> getChecked (from design-system/checkbox.tsx)', + 'core/bounties/config.ts -> useFeatureFlag (from core/state/feature-flags.ts)', ]); function sourceFiles(): string[] { @@ -149,11 +167,11 @@ function resolveImport(specifier: string, importingFile: string): string | null } /** - * `A` or `A as B` inside a named import block, skipping inline `type` specifiers. + * `A` or `A as B` inside a named block, skipping inline `type` specifiers. * * Both names are kept because they answer different questions. The export is known by its exported - * name, which is what the source module declares; it is used under its local one, which is what - * appears in JSX here. + * name, which is what the source module declares; it is referred to here by its local one, which is + * what a reader of this file sees and what belongs in a failure message. */ function parseNamedBindings(block: string): { exported: string; local: string }[] { return block @@ -167,39 +185,59 @@ function parseNamedBindings(block: string): { exported: string; local: string }[ .filter(({ exported, local }) => Boolean(exported) && Boolean(local)); } -/** - * Components are the one export a Server Component may take from a client module — that is what the - * boundary is for. So the question for every binding is whether it is one, and capitalisation alone - * cannot answer it: `BOARD_GRID_CLASS` fails a naive capital-letter test while being a string, and - * a client module exporting `const DefaultConfig = {...}` passes one while being an object. - * - * Three questions instead, cheapest first. - */ +/** Only a capitalised name can be a component. `useFeatureFlag` is a function and still a value. */ function isCapitalised(name: string): boolean { return /^[A-Z]/.test(name) && name !== name.toUpperCase(); } /** - * Whether the importing file treats it as a component: rendered as ``, or handed to something - * else to render as `render={Name}`. The sibling test matches render sites the same way. + * What the client module declares this export to be, read from the source rather than guessed from + * how it is used. * - * The prop form is here for a case the tree does not hold yet — a server module importing a client - * component and passing it on without rendering it — because the failure would otherwise be a - * false accusation, and a guard that cries wolf gets deleted. - */ -function usedAsComponent(local: string, contents: string): boolean { - return new RegExp(`<${local}[\\s/>]|=\\{\\s*${local}\\s*\\}`).test(contents); -} - -/** - * Whether the source module declares it as a type, which the compiler erases before anything runs. + * The first version of this asked the use site — rendered as ``, or handed on as + * `render={Name}`. That let `config={DefaultConfig}` through, since any prop looked like proof, and + * it had nothing to say about a re-export, which has no use site at all. The declaration answers + * both and is the same evidence a reader would use. * - * Two of these exist today and both would otherwise be reported: `Tabs` from `editor-provider` and - * `Feature` from `use-place-search` are `export type`, imported without the `type` keyword and used - * only in annotations. + * Measured against the tree before being trusted: of 163 capitalised imports from client modules in + * the server graph, 144 are `export function`, 17 are an arrow or `memo`/`forwardRef`, and 2 are + * `export type` imported without the `type` keyword — `Tabs` from `editor-provider` and `Feature` + * from `use-place-search`. Nothing is unclassifiable, so `unknown` is a real signal rather than the + * common case, and it is reported rather than waved through. */ -function declaredAsType(exported: string, sourceContents: string): boolean { - return new RegExp(`^export\\s+(?:type|interface)\\s+${exported}\\b`, 'm').test(sourceContents); +type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; + +function classifyExport(exported: string, source: string): ExportKind { + const name = exported.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + + if (new RegExp(`^export\\s+(?:type|interface)\\s+${name}\\b`, 'm').test(source)) return 'erased'; + + if (exported === 'default') { + if (/^export\s+default\s+(?:async\s+)?(?:function|class)\b/m.test(source)) return 'component'; + if (/^export\s+default\s+(?:\{|\[|['"`]|\d)/m.test(source)) return 'value'; + return 'unknown'; + } + + if (new RegExp(`^export\\s+(?:async\\s+)?function\\s+${name}\\b`, 'm').test(source)) return 'component'; + if (new RegExp(`^export\\s+class\\s+${name}\\b`, 'm').test(source)) return 'component'; + + const declaration = new RegExp(`^export\\s+const\\s+${name}\\s*(?::[^=]+)?=\\s*(.{0,40})`, 'm').exec(source); + if (declaration) { + const initialiser = declaration[1].trimStart(); + // A component, however it is wrapped. + if ( + /^(?:\(|async\s*\(|[A-Za-z_$][\w$]*\s*=>|React\.(?:memo|forwardRef)|memo\(|forwardRef\(|styled\.|cva\()/.test( + initialiser + ) + ) { + return 'component'; + } + // An object, array, string, number or boolean is a value whatever its name suggests. + if (/^(?:\{|\[|['"`]|\d|true\b|false\b|new\s)/.test(initialiser)) return 'value'; + return 'unknown'; + } + + return 'unknown'; } describe('server components take only components from client modules', () => { @@ -211,6 +249,10 @@ describe('server components take only components from client modules', () => { // Guards against a silently empty run if the layout moves. expect(files.length).toBeGreaterThan(50); expect(clientFiles.size).toBeGreaterThan(50); + // Every declared root must hold something, or a typo in the list reads as a clean run. + for (const dir of SOURCE_DIRS) { + expect(files.filter(file => file.startsWith(`${dir}${path.sep}`)).length).toBeGreaterThan(0); + } }); /** Every non-client module reachable from a server entry point, which is where this can bite. */ @@ -225,8 +267,8 @@ describe('server components take only components from client modules', () => { if (serverGraph.has(file) || clientFiles.has(file)) continue; serverGraph.add(file); - for (const match of (contentsByFile.get(file) ?? '').matchAll(MODULE_EDGE)) { - const target = resolveImport(match[1], file); + for (const specifier of runtimeSpecifiers(contentsByFile.get(file) ?? '')) { + const target = resolveImport(specifier, file); if (target && !clientFiles.has(target) && !serverGraph.has(target)) queue.push(target); } } @@ -235,93 +277,87 @@ describe('server components take only components from client modules', () => { expect(serverGraph.size).toBeGreaterThan(100); }); - /** - * Whether this binding is a component as far as this file is concerned — the one thing a server - * module may take across the boundary. Capitalised *and* either used as one here or erased by the - * compiler; capitalised and neither is the `const DefaultConfig = {...}` case, which is a value - * wearing a component's name. - */ - function isComponentHere({ - exported, - local, - contents, - target, - }: { - exported: string; - local: string; - contents: string; - target: string; - }): boolean { - if (!isCapitalised(exported === 'default' ? local : exported)) return false; - - return usedAsComponent(local, contents) || declaredAsType(exported, contentsByFile.get(target) ?? ''); - } - /** * Every value a server-graph module takes from a client module, in any of the shapes it can - * arrive in, as `[offence, source]` pairs. A namespace binding is reported whole, since every - * property read off it is a client reference. + * arrive in. A namespace binding is reported whole, since every property read off it is a client + * reference and there is no one export to classify. */ - function* clientValuesInServerGraph(): Generator<[string, string]> { + function* clientValuesInServerGraph(): Generator { for (const file of serverGraph) { const contents = contentsByFile.get(file)!; const from = file.split(path.sep).join('/'); - const fromClientModule = (specifier: string) => { + const clientSource = (specifier: string) => { const target = resolveImport(specifier, file); - return target && clientFiles.has(target) ? target.split(path.sep).join('/') : null; + if (!target || !clientFiles.has(target)) return null; + return { path: target.split(path.sep).join('/'), contents: contentsByFile.get(target) ?? '' }; }; - for (const [, block, specifier] of contents.matchAll(NAMED_IMPORT_BLOCK)) { - const target = fromClientModule(specifier); - if (!target) continue; + /** The offence, or nothing if this binding is a component or erased before it runs. */ + const offence = (exported: string, local: string, source: { path: string; contents: string }) => { + if (!isCapitalised(exported === 'default' ? local : exported)) { + return `${from} -> ${local} (from ${source.path})`; + } + + const kind = classifyExport(exported, source.contents); + if (kind === 'component' || kind === 'erased') return null; + + const label = kind === 'unknown' ? `${local} (unclassifiable` : `${local} (`; + return `${from} -> ${label}from ${source.path})`; + }; + + for (const [, block, specifier] of contents.matchAll(IMPORT_NAMED)) { + const source = clientSource(specifier); + if (!source) continue; for (const { exported, local } of parseNamedBindings(block)) { - if (isComponentHere({ exported, local, contents, target })) continue; - yield [`${from} -> ${exported === 'default' ? `default as ${local}` : exported}`, target]; + const found = offence(exported, local, source); + if (found) yield found; } } - for (const [, local, specifier] of contents.matchAll(DEFAULT_IMPORT)) { - const target = fromClientModule(specifier); - if (!target) continue; - if (isComponentHere({ exported: 'default', local, contents, target })) continue; - yield [`${from} -> default as ${local}`, target]; + for (const [, local, specifier] of contents.matchAll(IMPORT_DEFAULT)) { + const source = clientSource(specifier); + if (!source) continue; + const found = offence('default', local, source); + if (found) yield found; } - for (const [, local, specifier] of contents.matchAll(NAMESPACE_IMPORT)) { - const target = fromClientModule(specifier); - if (target) yield [`${from} -> * as ${local}`, target]; + for (const [, local, specifier] of contents.matchAll(IMPORT_NAMESPACE)) { + const source = clientSource(specifier); + if (source) yield `${from} -> * as ${local} (from ${source.path})`; } - for (const [, block, specifier] of contents.matchAll(NAMED_REEXPORT)) { - const target = fromClientModule(specifier); - if (!target) continue; - for (const { exported } of parseNamedBindings(block)) { - // No JSX to look at in a re-export, so capitalisation and the type check are all there is. - if (isCapitalised(exported) || declaredAsType(exported, contentsByFile.get(target) ?? '')) continue; - yield [`${from} -> re-exports ${exported}`, target]; + for (const [, block, specifier] of contents.matchAll(REEXPORT_NAMED)) { + const source = clientSource(specifier); + if (!source) continue; + for (const { exported, local } of parseNamedBindings(block)) { + const found = offence(exported, local, source); + if (found) yield `${found} re-exported`; } } - for (const [, namespace, specifier] of contents.matchAll(STAR_REEXPORT)) { - const target = fromClientModule(specifier); - if (target) yield [`${from} -> re-exports ${namespace ? `* as ${namespace}` : '*'}`, target]; + for (const [, namespace, specifier] of contents.matchAll(REEXPORT_STAR)) { + const source = clientSource(specifier); + if (source) yield `${from} -> re-exports ${namespace ? `* as ${namespace}` : '*'} (from ${source.path})`; } } } it('finds no non-component value taken from a "use client" module', () => { - const offences = [...clientValuesInServerGraph()] - .filter(([offence]) => !KNOWN.has(offence)) - .map(([offence, target]) => `${offence} (from ${target})`); - - expect(offences).toEqual([]); + const offences = [...clientValuesInServerGraph()].filter(offence => !KNOWN.has(offence)); + + expect( + offences, + 'A server component is reading a value out of a client module, which is a client reference ' + + 'on the server rather than the value. Move the value into a module with no `use client` ' + + 'and import it from both sides — see the doc block in this file.' + ).toEqual([]); }); it('keeps the known list honest', () => { // An entry that no longer matches anything has been fixed, and leaving it here would quietly // re-permit the same import later. - const live = new Set([...clientValuesInServerGraph()].map(([offence]) => offence)); + const live = new Set(clientValuesInServerGraph()); expect([...KNOWN].filter(entry => !live.has(entry))).toEqual([]); }); From 80e6031f583214cd4f1dea1ab5b7b87df8650aea Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 13:22:26 -0700 Subject: [PATCH 04/23] test: read declarations with the compiler instead of matching near them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four more findings, all four real and all four in the classifier from the round before. That is the finding: this was the third round of patching the same regexes, and every question they were failing at — what is a directive, what is a class, what does an identifier resolve to, what does a parenthesis contain — is a question about syntax that the compiler already answers. So it parses now. `ts.createSourceFile` over 1,600 files, top-level statements only, and the classifier reads declarations rather than the text near them. Each of the four, planted before and after: - `'use client'` behind a licence header. The regex saw the first token only, so a real client module went unrecognised — and that is worse than unchecked, because it joins the server graph and everything it exports stops being an offence anywhere. Proved exactly that: with the header, the old version missed the planted import and started reporting the module's own imports instead. - `export default SuggestedFormats`, a component declared above and exported by name, in a real file. Reported as unclassifiable before, silent now — the initialiser is followed through an identifier, and through parentheses, `as` and `satisfies` too. - `export class GeoChatRequestError extends Error`, also a real file. A capital letter made it a component; heritage decides it now, so only React's base classes qualify. - `export const DefaultConfig = ({ enabled: true })` and `export const ButtonStyles = cva('x')`. A leading bracket and a call both read as components before. A call is a value unless it is `memo` or `forwardRef`. A fifth test pins the classifier's shape against the tree: of the capitalised exports the server graph takes from client modules, 100+ are components, exactly 2 are `export type` imported without the keyword, and none is a value or unreadable. A change that started calling components values moves that before it reaches the allowlist. Costs 2.3s against 0.5s, nearly all of it loading the compiler. One thing still read from the text: `import('literal')`, because finding those in the tree means walking every node of every file and a computed specifier resolves to nothing anyway. --- ...client-values-in-server-boundaries.test.ts | 543 ++++++++++++------ 1 file changed, 361 insertions(+), 182 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 1f90f4ac4e..5d30a5f813 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -1,7 +1,8 @@ // This walks the source tree with `fs` and never touches the DOM. // @vitest-environment node -import { existsSync, readFileSync, readdirSync } from 'node:fs'; +import { readFileSync, readdirSync } from 'node:fs'; import path from 'node:path'; +import ts from 'typescript'; import { describe, expect, it } from 'vitest'; /** @@ -19,14 +20,30 @@ import { describe, expect, it } from 'vitest'; * The sibling of this test guards the other direction — async components rendered from client * files. Same failure mode: correct-looking UI, wrong boundary. * - * Two things it deliberately cannot see, both of which make the allowlist a list of things checked - * by hand rather than a list of things that are fine: + * ## Why this parses instead of matching + * + * It used to match patterns near declarations rather than read them, and review found ten ways that + * was wrong. An unbounded matcher began on a bare CSS import and captured a later statement's + * specifier. `import Default, * as Namespace` matched neither of two patterns. `'use client'` was + * recognised only as the very first token, so a licence header would hide an entire client module — + * and an unrecognised client module is worse than an unchecked one, because it joins the server + * graph and everything it exports stops being an offence anywhere. `export default + * SuggestedFormats`, a component declared above and exported by name, came out unclassifiable. + * `export class GeoChatRequestError extends Error` was accepted as a component for being + * capitalised. `export const DefaultConfig = ({ enabled: true })` was accepted for opening with a + * bracket. + * + * Every one of those is a question about syntax, and the compiler answers questions about syntax. + * Parsing 1,600 files costs ~600ms, which is less than the patterns cost in review rounds. + * + * Two things it still cannot see, which is what keeps the allowlist a list of things checked by + * hand rather than a list of things that are fine: * * 1. Whether the value is ever *read* while rendering on the server. A client hook sitting next to * a server-safe constant and only ever called from a client component is inert. * 2. What a dynamically imported module's bindings are. The edge is followed, so everything beyond - * it is still guarded, but `const { x } = await import('./client')` is not destructured here. - * No server-graph module does that today. + * it stays guarded, but `const { x } = await import('./client')` is not destructured. No + * server-graph module does that today. */ const ROOT = path.resolve(__dirname, '..', '..'); @@ -48,47 +65,13 @@ const SOURCE_DIRS = ['app', 'atoms', 'core', 'design-system', 'partials']; const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; /** - * One matcher per declaration shape, rather than one loose pattern for all of them. - * - * The loose version was `^(?:import|export)\s+(?!type\s)[\s\S]*?from\s+['"](…)['"]`, and `[\s\S]*?` - * does not stop at the end of a declaration. In `app/layout.tsx` it started on a bare - * `import 'katex/dist/katex.min.css';`, walked past two more statements and captured the specifier - * of a later one — so edges were attributed to declarations that do not have them, and any - * statement in between was skipped because the match had already consumed it. It would equally - * start on an `export const` and run until it found a `from` dozens of lines away. + * `import('…')` with a literal specifier — the one thing still read from the text. * - * Each of these is bounded to its own shape: a `{…}` block cannot contain a `}`, and nothing else - * crosses a declaration boundary. Between them they cover what the tree actually uses — ~9800 named - * imports, ~340 default, ~870 namespace, 53 re-exports, 136 bare, 85 dynamic. - * - * `import type` and `export type` are excluded, and that exclusion is load-bearing: the only route - * into `core/blocks/data/filters.ts` is a type import from `core/chat/edit-types.ts`, and counting - * it walks on into the sync store and reports three modules TypeScript erases before anything runs. + * Finding these in the tree means walking every node of every file rather than its top-level + * statements, and a dynamic import with a computed specifier resolves to no file anyway. + * Declarations, where every mistake has been, are parsed. */ -const IMPORT_NAMED = /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; -const IMPORT_DEFAULT = - /^import\s+(?!type\s)([A-Za-z_$][\w$]*)\s*(?:,\s*(?:\{[^}]*\}|\*\s+as\s+[A-Za-z_$][\w$]*))?\s+from\s+['"]([^'"]+)['"]/gm; -const IMPORT_NAMESPACE = - /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+['"]([^'"]+)['"]/gm; -const REEXPORT_NAMED = /^export\s+(?!type\s)\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; -const REEXPORT_STAR = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"]([^'"]+)['"]/gm; -const IMPORT_BARE = /^import\s+['"]([^'"]+)['"]/gm; -const IMPORT_DYNAMIC = /\bimport\(\s*['"]([^'"]+)['"]\s*\)/g; - -/** Every specifier a module pulls in at runtime, whatever shape the declaration took. */ -function runtimeSpecifiers(contents: string): string[] { - const found: string[] = []; - - for (const [, , specifier] of contents.matchAll(IMPORT_NAMED)) found.push(specifier); - for (const [, , specifier] of contents.matchAll(IMPORT_DEFAULT)) found.push(specifier); - for (const [, , specifier] of contents.matchAll(IMPORT_NAMESPACE)) found.push(specifier); - for (const [, , specifier] of contents.matchAll(REEXPORT_NAMED)) found.push(specifier); - for (const [, , specifier] of contents.matchAll(REEXPORT_STAR)) found.push(specifier); - for (const [, specifier] of contents.matchAll(IMPORT_BARE)) found.push(specifier); - for (const [, specifier] of contents.matchAll(IMPORT_DYNAMIC)) found.push(specifier); - - return found; -} +const DYNAMIC_IMPORT = /\bimport\(\s*['"]([^'"]+)['"]\s*\)/g; /** * What the tree holds today, each one read before being listed rather than swept up by the walk. @@ -138,112 +121,299 @@ function sourceFiles(): string[] { return found; } -function isClientFile(contents: string): boolean { - return /^\s*['"]use client['"]/.test(contents); -} - -function resolveImport(specifier: string, importingFile: string): string | null { - let absolute: string; - - if (specifier.startsWith('~/')) { - absolute = path.join(ROOT, specifier.slice(2)); - } else if (specifier.startsWith('.')) { - absolute = path.resolve(ROOT, path.dirname(importingFile), specifier); - } else { - return null; - } - - const relative = path.relative(ROOT, absolute); - for (const candidate of [ - `${relative}.tsx`, - `${relative}.ts`, - path.join(relative, 'index.tsx'), - path.join(relative, 'index.ts'), - ]) { - if (!SOURCE_DIRS.some(dir => candidate.startsWith(`${dir}${path.sep}`))) continue; - if (existsSync(path.join(ROOT, candidate))) return candidate; - } - return null; +function parse(file: string, contents: string): ts.SourceFile { + return ts.createSourceFile( + file, + contents, + ts.ScriptTarget.Latest, + true, + file.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS + ); } /** - * `A` or `A as B` inside a named block, skipping inline `type` specifiers. + * Whether the module opts into the client, read from its directive prologue. * - * Both names are kept because they answer different questions. The export is known by its exported - * name, which is what the source module declares; it is referred to here by its local one, which is - * what a reader of this file sees and what belongs in a failure message. + * A directive is a leading statement whose expression is a plain string, and comments are trivia + * rather than statements — so a licence header or a `'use strict';` in front of `'use client'` no + * longer hides the module, which matching the first token of the file did. */ -function parseNamedBindings(block: string): { exported: string; local: string }[] { - return block - .split(',') - .map(entry => entry.trim()) - .filter(entry => entry.length > 0 && !entry.startsWith('type ')) - .map(entry => { - const [exported, local] = entry.split(/\s+as\s+/).map(part => part.trim()); - return { exported, local: local ?? exported }; - }) - .filter(({ exported, local }) => Boolean(exported) && Boolean(local)); +function isClientModule(sourceFile: ts.SourceFile): boolean { + for (const statement of sourceFile.statements) { + if (!ts.isExpressionStatement(statement) || !ts.isStringLiteralLike(statement.expression)) return false; + if (statement.expression.text === 'use client') return true; + } + return false; } -/** Only a capitalised name can be a component. `useFeatureFlag` is a function and still a value. */ -function isCapitalised(name: string): boolean { - return /^[A-Z]/.test(name) && name !== name.toUpperCase(); +function hasModifier(node: ts.Node, kind: ts.SyntaxKind): boolean { + return (ts.canHaveModifiers(node) ? (ts.getModifiers(node) ?? []) : []).some(modifier => modifier.kind === kind); +} + +const isExported = (node: ts.Node) => hasModifier(node, ts.SyntaxKind.ExportKeyword); +const isDefault = (node: ts.Node) => hasModifier(node, ts.SyntaxKind.DefaultKeyword); + +/** What a client module's export turns out to be, once its declaration is read. */ +type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; + +/** `memo(X)` and `forwardRef(X)`, plain or `React.`-qualified, produce components. Nothing else does. */ +const COMPONENT_WRAPPERS = new Set(['memo', 'forwardRef']); + +function isComponentWrapper(expression: ts.Expression): boolean { + if (ts.isIdentifier(expression)) return COMPONENT_WRAPPERS.has(expression.text); + if (ts.isPropertyAccessExpression(expression)) return COMPONENT_WRAPPERS.has(expression.name.text); + return false; } /** - * What the client module declares this export to be, read from the source rather than guessed from - * how it is used. - * - * The first version of this asked the use site — rendered as ``, or handed on as - * `render={Name}`. That let `config={DefaultConfig}` through, since any prop looked like proof, and - * it had nothing to say about a re-export, which has no use site at all. The declaration answers - * both and is the same evidence a reader would use. + * A class is a component only if it extends React's. * - * Measured against the tree before being trusted: of 163 capitalised imports from client modules in - * the server graph, 144 are `export function`, 17 are an arrow or `memo`/`forwardRef`, and 2 are - * `export type` imported without the `type` keyword — `Tabs` from `editor-provider` and `Feature` - * from `use-place-search`. Nothing is unclassifiable, so `unknown` is a real signal rather than the - * common case, and it is reported rather than waved through. + * `export class GeoChatRequestError extends Error` lives in a `'use client'` module and a capital + * letter alone let it through. An Error subclass read on the server is a client reference like any + * other value. */ -type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; +function classifyClass(node: ts.ClassLikeDeclaration): ExportKind { + const extended = (node.heritageClauses ?? []) + .filter(clause => clause.token === ts.SyntaxKind.ExtendsKeyword) + .flatMap(clause => clause.types.map(type => type.expression.getText())); + + return extended.some(name => /(^|\.)(Pure)?Component$/.test(name)) ? 'component' : 'value'; +} + +describe('server components take only components from client modules', () => { + const files = sourceFiles(); + const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); + const astByFile = new Map([...contentsByFile].map(([file, contents]) => [file, parse(file, contents)])); + const clientFiles = new Set([...astByFile].filter(([, ast]) => isClientModule(ast)).map(([file]) => file)); + + const exportKinds = new Map>(); + + /** + * What each of a module's exports is, by exported name, with `default` keyed as `default`. + * + * An initialiser is followed where following it answers the question: through parentheses, `as` + * and `satisfies`, and through a local identifier — which is how `export default SuggestedFormats` + * reaches the arrow function declared above it instead of giving up and reporting a real + * component. + */ + function kindsFor(file: string): Map { + const cached = exportKinds.get(file); + if (cached) return cached; + + const sourceFile = astByFile.get(file)!; + const kinds = new Map(); + /** Local declarations, so an export by identifier has something to resolve against. */ + const locals = new Map(); + + for (const statement of sourceFile.statements) { + if (ts.isVariableStatement(statement)) { + for (const declaration of statement.declarationList.declarations) { + if (ts.isIdentifier(declaration.name) && declaration.initializer) { + locals.set(declaration.name.text, declaration.initializer); + } + } + } else if ((ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) && statement.name) { + locals.set(statement.name.text, statement); + } + } + + const classify = (node: ts.Node, seen = new Set()): ExportKind => { + if (seen.has(node)) return 'unknown'; + seen.add(node); + + if (ts.isFunctionDeclaration(node) || ts.isArrowFunction(node) || ts.isFunctionExpression(node)) { + return 'component'; + } + if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node); + // `styled.div\`…\`` and friends. + if (ts.isTaggedTemplateExpression(node)) return 'component'; + if (ts.isCallExpression(node)) return isComponentWrapper(node.expression) ? 'component' : 'value'; + if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { + return classify(node.expression, seen); + } + if (ts.isIdentifier(node)) { + const local = locals.get(node.text); + return local ? classify(local, seen) : 'unknown'; + } + if ( + ts.isObjectLiteralExpression(node) || + ts.isArrayLiteralExpression(node) || + ts.isStringLiteralLike(node) || + ts.isNumericLiteral(node) || + ts.isNewExpression(node) || + node.kind === ts.SyntaxKind.TrueKeyword || + node.kind === ts.SyntaxKind.FalseKeyword + ) { + return 'value'; + } + return 'unknown'; + }; + + for (const statement of sourceFile.statements) { + if (ts.isTypeAliasDeclaration(statement) || ts.isInterfaceDeclaration(statement)) { + if (isExported(statement)) kinds.set(statement.name.text, 'erased'); + continue; + } + + // An enum is an object at runtime, whatever it looks like in the types. + if (ts.isEnumDeclaration(statement) && isExported(statement)) { + kinds.set(statement.name.text, 'value'); + continue; + } + + if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) { + if (!isExported(statement)) continue; + kinds.set(isDefault(statement) ? 'default' : (statement.name?.text ?? 'default'), classify(statement)); + continue; + } -function classifyExport(exported: string, source: string): ExportKind { - const name = exported.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + if (ts.isVariableStatement(statement) && isExported(statement)) { + for (const declaration of statement.declarationList.declarations) { + if (!ts.isIdentifier(declaration.name)) continue; + kinds.set(declaration.name.text, declaration.initializer ? classify(declaration.initializer) : 'unknown'); + } + continue; + } + + // `export default `, a bare identifier included. + if (ts.isExportAssignment(statement) && !statement.isExportEquals) { + kinds.set('default', classify(statement.expression)); + continue; + } - if (new RegExp(`^export\\s+(?:type|interface)\\s+${name}\\b`, 'm').test(source)) return 'erased'; + // `export { a }` and `export { a } from '…'`: resolvable only when declared here. + if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamedExports(statement.exportClause)) { + for (const element of statement.exportClause.elements) { + if (statement.isTypeOnly || element.isTypeOnly) { + kinds.set(element.name.text, 'erased'); + continue; + } + const local = locals.get((element.propertyName ?? element.name).text); + kinds.set(element.name.text, local ? classify(local) : 'unknown'); + } + } + } - if (exported === 'default') { - if (/^export\s+default\s+(?:async\s+)?(?:function|class)\b/m.test(source)) return 'component'; - if (/^export\s+default\s+(?:\{|\[|['"`]|\d)/m.test(source)) return 'value'; - return 'unknown'; + exportKinds.set(file, kinds); + return kinds; } - if (new RegExp(`^export\\s+(?:async\\s+)?function\\s+${name}\\b`, 'm').test(source)) return 'component'; - if (new RegExp(`^export\\s+class\\s+${name}\\b`, 'm').test(source)) return 'component'; - - const declaration = new RegExp(`^export\\s+const\\s+${name}\\s*(?::[^=]+)?=\\s*(.{0,40})`, 'm').exec(source); - if (declaration) { - const initialiser = declaration[1].trimStart(); - // A component, however it is wrapped. - if ( - /^(?:\(|async\s*\(|[A-Za-z_$][\w$]*\s*=>|React\.(?:memo|forwardRef)|memo\(|forwardRef\(|styled\.|cva\()/.test( - initialiser - ) - ) { - return 'component'; + function resolveImport(specifier: string, importingFile: string): string | null { + let absolute: string; + + if (specifier.startsWith('~/')) { + absolute = path.join(ROOT, specifier.slice(2)); + } else if (specifier.startsWith('.')) { + absolute = path.resolve(ROOT, path.dirname(importingFile), specifier); + } else { + return null; + } + + const relative = path.relative(ROOT, absolute); + for (const candidate of [ + `${relative}.tsx`, + `${relative}.ts`, + path.join(relative, 'index.tsx'), + path.join(relative, 'index.ts'), + ]) { + if (contentsByFile.has(candidate)) return candidate; } - // An object, array, string, number or boolean is a value whatever its name suggests. - if (/^(?:\{|\[|['"`]|\d|true\b|false\b|new\s)/.test(initialiser)) return 'value'; - return 'unknown'; + return null; } - return 'unknown'; -} + /** One binding taken from another module: a named export, a default, or a whole namespace. */ + type Reference = { + specifier: string; + exported?: string; + local: string; + namespace?: boolean; + reexported?: boolean; + }; -describe('server components take only components from client modules', () => { - const files = sourceFiles(); - const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); - const clientFiles = new Set([...contentsByFile].filter(([, c]) => isClientFile(c)).map(([f]) => f)); + const referencesByFile = new Map(); + + /** + * Every module a file pulls in at runtime, with the bindings it takes from each. + * + * Type-only imports and exports are skipped, whole-statement and per-element alike. That + * exclusion is load-bearing rather than tidy: the only route into `core/blocks/data/filters.ts` + * is a type import from `core/chat/edit-types.ts`, and following it walks on into the sync store + * and reports three modules TypeScript erases before anything runs. + */ + function references(file: string): Reference[] { + const cached = referencesByFile.get(file); + if (cached) return cached; + + const sourceFile = astByFile.get(file)!; + const found: Reference[] = []; + + for (const statement of sourceFile.statements) { + if (ts.isImportDeclaration(statement) && ts.isStringLiteralLike(statement.moduleSpecifier)) { + const specifier = statement.moduleSpecifier.text; + const clause = statement.importClause; + + // A bare `import './x'` takes no bindings but still loads the module. + if (!clause) { + found.push({ specifier, local: '' }); + continue; + } + if (clause.isTypeOnly) continue; + + if (clause.name) found.push({ specifier, exported: 'default', local: clause.name.text }); + + if (clause.namedBindings && ts.isNamespaceImport(clause.namedBindings)) { + found.push({ specifier, local: `* as ${clause.namedBindings.name.text}`, namespace: true }); + } else if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { + for (const element of clause.namedBindings.elements) { + if (element.isTypeOnly) continue; + found.push({ + specifier, + exported: (element.propertyName ?? element.name).text, + local: element.name.text, + }); + } + } + continue; + } + + if ( + ts.isExportDeclaration(statement) && + statement.moduleSpecifier && + ts.isStringLiteralLike(statement.moduleSpecifier) + ) { + if (statement.isTypeOnly) continue; + const specifier = statement.moduleSpecifier.text; + + if (!statement.exportClause) { + found.push({ specifier, local: 're-exports *', namespace: true, reexported: true }); + } else if (ts.isNamespaceExport(statement.exportClause)) { + found.push({ + specifier, + local: `re-exports * as ${statement.exportClause.name.text}`, + namespace: true, + reexported: true, + }); + } else { + for (const element of statement.exportClause.elements) { + if (element.isTypeOnly) continue; + found.push({ + specifier, + exported: (element.propertyName ?? element.name).text, + local: element.name.text, + reexported: true, + }); + } + } + } + } + + for (const [, specifier] of (contentsByFile.get(file) ?? '').matchAll(DYNAMIC_IMPORT)) { + found.push({ specifier, local: '' }); + } + + referencesByFile.set(file, found); + return found; + } it('walks a source tree that actually has files in it', () => { // Guards against a silently empty run if the layout moves. @@ -267,7 +437,7 @@ describe('server components take only components from client modules', () => { if (serverGraph.has(file) || clientFiles.has(file)) continue; serverGraph.add(file); - for (const specifier of runtimeSpecifiers(contentsByFile.get(file) ?? '')) { + for (const { specifier } of references(file)) { const target = resolveImport(specifier, file); if (target && !clientFiles.has(target) && !serverGraph.has(target)) queue.push(target); } @@ -277,72 +447,58 @@ describe('server components take only components from client modules', () => { expect(serverGraph.size).toBeGreaterThan(100); }); + /** Only a capitalised name can be a component. `useFeatureFlag` is a function and still a value. */ + function isCapitalised(name: string): boolean { + return /^[A-Z]/.test(name) && name !== name.toUpperCase(); + } + /** - * Every value a server-graph module takes from a client module, in any of the shapes it can - * arrive in. A namespace binding is reported whole, since every property read off it is a client - * reference and there is no one export to classify. + * Every binding a server-graph module takes from a client module, with what the source says it + * is. A name that is not capitalised cannot be a component whatever its declaration says, so it + * is reported without asking. */ - function* clientValuesInServerGraph(): Generator { + function* clientBindings(): Generator<{ offence: string; kind: ExportKind; capitalised: boolean }> { for (const file of serverGraph) { - const contents = contentsByFile.get(file)!; const from = file.split(path.sep).join('/'); - const clientSource = (specifier: string) => { - const target = resolveImport(specifier, file); - if (!target || !clientFiles.has(target)) return null; - return { path: target.split(path.sep).join('/'), contents: contentsByFile.get(target) ?? '' }; - }; + for (const reference of references(file)) { + const target = resolveImport(reference.specifier, file); + if (!target || !clientFiles.has(target)) continue; - /** The offence, or nothing if this binding is a component or erased before it runs. */ - const offence = (exported: string, local: string, source: { path: string; contents: string }) => { - if (!isCapitalised(exported === 'default' ? local : exported)) { - return `${from} -> ${local} (from ${source.path})`; - } - - const kind = classifyExport(exported, source.contents); - if (kind === 'component' || kind === 'erased') return null; - - const label = kind === 'unknown' ? `${local} (unclassifiable` : `${local} (`; - return `${from} -> ${label}from ${source.path})`; - }; + const source = target.split(path.sep).join('/'); + const suffix = reference.reexported && !reference.namespace ? ' re-exported' : ''; - for (const [, block, specifier] of contents.matchAll(IMPORT_NAMED)) { - const source = clientSource(specifier); - if (!source) continue; - for (const { exported, local } of parseNamedBindings(block)) { - const found = offence(exported, local, source); - if (found) yield found; + // A namespace has no one export to read, and every property taken off it is a reference. + if (reference.namespace) { + yield { offence: `${from} -> ${reference.local} (from ${source})`, kind: 'value', capitalised: false }; + continue; } - } - - for (const [, local, specifier] of contents.matchAll(IMPORT_DEFAULT)) { - const source = clientSource(specifier); - if (!source) continue; - const found = offence('default', local, source); - if (found) yield found; - } - - for (const [, local, specifier] of contents.matchAll(IMPORT_NAMESPACE)) { - const source = clientSource(specifier); - if (source) yield `${from} -> * as ${local} (from ${source.path})`; - } - - for (const [, block, specifier] of contents.matchAll(REEXPORT_NAMED)) { - const source = clientSource(specifier); - if (!source) continue; - for (const { exported, local } of parseNamedBindings(block)) { - const found = offence(exported, local, source); - if (found) yield `${found} re-exported`; + if (!reference.exported) continue; + + const named = reference.exported === 'default' ? reference.local : reference.exported; + if (!isCapitalised(named)) { + yield { + offence: `${from} -> ${reference.local} (from ${source})${suffix}`, + kind: 'value', + capitalised: false, + }; + continue; } - } - for (const [, namespace, specifier] of contents.matchAll(REEXPORT_STAR)) { - const source = clientSource(specifier); - if (source) yield `${from} -> re-exports ${namespace ? `* as ${namespace}` : '*'} (from ${source.path})`; + const kind = kindsFor(target).get(reference.exported) ?? 'unknown'; + const label = kind === 'unknown' ? `${reference.local} (unclassifiable` : `${reference.local} (`; + yield { offence: `${from} -> ${label}from ${source})${suffix}`, kind, capitalised: true }; } } } + function* clientValuesInServerGraph(): Generator { + for (const { offence, kind } of clientBindings()) { + if (kind === 'component' || kind === 'erased') continue; + yield offence; + } + } + it('finds no non-component value taken from a "use client" module', () => { const offences = [...clientValuesInServerGraph()].filter(offence => !KNOWN.has(offence)); @@ -361,4 +517,27 @@ describe('server components take only components from client modules', () => { expect([...KNOWN].filter(entry => !live.has(entry))).toEqual([]); }); + + /** + * The classifier, measured against the tree rather than against invented examples. + * + * The guard above is green, so every capitalised export it lets through has to be a component or + * erased — meaning a change to `classify` that started calling components values, or values + * components, would move these counts. Pinning the shape is what makes that visible rather than + * silent, and it is the assertion that would have caught `export class … extends Error` being + * waved through for its capital letter. + */ + it('classifies every capitalised client export the server graph takes', () => { + const kinds: Record = { component: 0, erased: 0, value: 0, unknown: 0 }; + for (const { kind, capitalised } of clientBindings()) if (capitalised) kinds[kind] += 1; + + // Components, and the two `export type`s imported without the `type` keyword — `Tabs` from + // `editor-provider` and `Feature` from `use-place-search`. + expect(kinds.component).toBeGreaterThan(100); + expect(kinds.erased).toBe(2); + // Nothing capitalised is a value or unreadable. A classifier that started calling components + // values would move this off zero before the offence list above grew past its allowlist. + expect(kinds.value).toBe(0); + expect(kinds.unknown).toBe(0); + }); }); From b000679d2e89674773db470524202d8ed99328db Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 14:01:23 -0700 Subject: [PATCH 05/23] test: seed metadata routes, and stop guessing what a capitalised name is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings. Four implemented as asked, one implemented the other way round because measuring says the suggested fix fails the build on master. Seeded: the metadata routes. `robots`, `sitemap`, `manifest`, `icon`, `apple-icon` and `twitter-image` are modules Next runs on the server as much as a page is, and `app/robots.ts` is already in this tree, so anything reachable only through it sat outside the walk. Planted a value import there — silent before, caught now. A directive is a string literal. `ts.isStringLiteralLike` also accepts a no-substitution template literal, so a file opening `` `use client` `` was taken for a client module; planted one and the old version raised a false offence against it. Module specifiers get the same stricter predicate, where a template literal is not legal either. A tagged template is a value. `gql`, `css` and `sql` produce data, and `styled.div` — the tag that would produce a component — is used nowhere in this repo; its only mention was the comment justifying the exemption. `export const SpaceQuery = gql\`…\`` goes through before and is caught now. Dynamic imports are read from the tree. The regex matched inside comments, and would have matched `type T = import('./types').T`; planted the comment case and the old version followed it into a module nothing imports. A `CallExpression` whose callee is the `import` keyword cannot be either. 86ms for the walk, and nothing in this file reads source text any more. The exception is capitalised functions. Review asked for evidence of renderability, or `unknown` without it. Of the 520 capitalised functions client modules export, 505 contain JSX and 15 do not — and all 15 are components that render nothing and only run effects. Six are imported by the server graph today, so that rule would accuse `SpaceRedirect`, `PersonalProfileSuggestedTaskSync` and `PersonalProfileBioStarterMerge` of being values, and a guard that accuses real components is one somebody deletes. The evidence runs the other way instead: every `return` handing back an object, array, string or number makes it a value, which catches `function BuildOptions() { return {}; }` — the case review named — and leaves the effect-only components alone. --- ...client-values-in-server-boundaries.test.ts | 120 +++++++++++++++--- 1 file changed, 102 insertions(+), 18 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 5d30a5f813..6f1163b419 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -34,9 +34,10 @@ import { describe, expect, it } from 'vitest'; * bracket. * * Every one of those is a question about syntax, and the compiler answers questions about syntax. - * Parsing 1,600 files costs ~600ms, which is less than the patterns cost in review rounds. + * Parsing 1,600 files costs ~600ms and walking every node of them another 86ms, which is less than + * the patterns cost in review rounds. Nothing here is read from the source text any more. * - * Two things it still cannot see, which is what keeps the allowlist a list of things checked by + * Three things it still cannot see, which is what keeps the allowlist a list of things checked by * hand rather than a list of things that are fine: * * 1. Whether the value is ever *read* while rendering on the server. A client hook sitting next to @@ -44,6 +45,8 @@ import { describe, expect, it } from 'vitest'; * 2. What a dynamically imported module's bindings are. The edge is followed, so everything beyond * it stays guarded, but `const { x } = await import('./client')` is not destructured. No * server-graph module does that today. + * 3. Whether a capitalised function returning a non-literal value is a component. It is assumed to + * be one — see `classifyFunction` for why that default is the safe one here. */ const ROOT = path.resolve(__dirname, '..', '..'); @@ -61,17 +64,41 @@ const ROOT = path.resolve(__dirname, '..', '..'); */ const SOURCE_DIRS = ['app', 'atoms', 'core', 'design-system', 'partials']; -/** The files Next renders on the server by definition. Everything they reach is the server graph. */ -const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; +/** + * The files Next runs on the server by definition. Everything they reach is the server graph. + * + * The metadata routes belong here as much as the pages do — they are modules Next executes — and + * `app/robots.ts` is already in this tree, so a client value reachable only through it was outside + * the walk entirely. + */ +const SERVER_ENTRY = + /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image|robots|sitemap|manifest|icon|apple-icon|twitter-image)\.tsx?$/; /** - * `import('…')` with a literal specifier — the one thing still read from the text. + * The specifiers of `import('…')` calls, found in the tree rather than in the text. + * + * Reading these off the source matched them inside comments, strings and template literals, and + * matched `type T = import('./types').T` — an `ImportTypeNode`, which TypeScript erases — as a + * runtime edge. It also missed any spelling with a comment in the middle. A `CallExpression` whose + * callee is the `import` keyword is none of those things by construction. * - * Finding these in the tree means walking every node of every file rather than its top-level - * statements, and a dynamic import with a computed specifier resolves to no file anyway. - * Declarations, where every mistake has been, are parsed. + * Walking every node of all 1,633 files costs 86ms, which was the only argument for the regex. */ -const DYNAMIC_IMPORT = /\bimport\(\s*['"]([^'"]+)['"]\s*\)/g; +function dynamicImportSpecifiers(sourceFile: ts.SourceFile): string[] { + const found: string[] = []; + + const visit = (node: ts.Node) => { + if (ts.isCallExpression(node) && node.expression.kind === ts.SyntaxKind.ImportKeyword) { + const [specifier] = node.arguments; + // A computed specifier resolves to no file, so there is nothing to follow. + if (specifier && ts.isStringLiteral(specifier)) found.push(specifier.text); + } + ts.forEachChild(node, visit); + }; + ts.forEachChild(sourceFile, visit); + + return found; +} /** * What the tree holds today, each one read before being listed rather than swept up by the walk. @@ -140,7 +167,10 @@ function parse(file: string, contents: string): ts.SourceFile { */ function isClientModule(sourceFile: ts.SourceFile): boolean { for (const statement of sourceFile.statements) { - if (!ts.isExpressionStatement(statement) || !ts.isStringLiteralLike(statement.expression)) return false; + // `ts.isStringLiteral`, not `isStringLiteralLike`: that also accepts a no-substitution template + // literal, and `` `use client` `` is not a directive — treating it as one would move a server + // module into `clientFiles` and stop the walk at it. + if (!ts.isExpressionStatement(statement) || !ts.isStringLiteral(statement.expression)) return false; if (statement.expression.text === 'use client') return true; } return false; @@ -156,6 +186,16 @@ const isDefault = (node: ts.Node) => hasModifier(node, ts.SyntaxKind.DefaultKeyw /** What a client module's export turns out to be, once its declaration is read. */ type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; +/** + * A tagged template is a value here. + * + * `gql`, `css` and `sql` produce data, and `styled.div\`…\`` — the one tag that would produce a + * component — is not used anywhere in this repo (its only mention was this comment). Classifying + * every tagged template as a component to accommodate a library nobody imports is a hole in + * exchange for nothing. If styled-components ever arrives, the guard fires and someone adds the + * tag, which is a visible failure rather than a silent one. + */ + /** `memo(X)` and `forwardRef(X)`, plain or `React.`-qualified, produce components. Nothing else does. */ const COMPONENT_WRAPPERS = new Set(['memo', 'forwardRef']); @@ -165,6 +205,53 @@ function isComponentWrapper(expression: ts.Expression): boolean { return false; } +/** + * A capitalised function is a component unless it is caught returning something else. + * + * Review asked for the opposite default — evidence that a function is renderable, or `unknown` when + * there is none — and measuring says that would fail the build on `master` today. Of the 520 + * capitalised functions client modules export, 505 contain JSX and 15 do not, and all 15 are real + * components that render nothing and only run effects: `DeepLinkHandler`, `SentryUserIdentifier`, + * `PendingActionsRunner` and so on. Six of those 15 are imported by the server graph right now, so + * demanding JSX would accuse `SpaceRedirect`, `PersonalProfileSuggestedTaskSync` and + * `PersonalProfileBioStarterMerge` of being values. A guard that accuses real components is one + * somebody deletes. + * + * So the evidence runs the other way: a function whose every `return` hands back an object, array, + * string or number is a value — which is `function BuildOptions() { return {}; }`, the case review + * named — and anything else is left as a component. That leaves a residue, a function returning a + * value through a variable rather than a literal, and it is the right residue to have: silent where + * it is unsure, loud only where it is certain. + */ +function classifyFunction(node: ts.SignatureDeclaration): ExportKind { + const returned: ts.Node[] = []; + let rendersJsx = false; + + const visit = (child: ts.Node) => { + // A nested function's returns are its own, not this one's. + if ( + child !== node && + (ts.isFunctionDeclaration(child) || ts.isArrowFunction(child) || ts.isFunctionExpression(child)) + ) { + return; + } + if (ts.isJsxElement(child) || ts.isJsxSelfClosingElement(child) || ts.isJsxFragment(child)) rendersJsx = true; + if (ts.isReturnStatement(child) && child.expression) returned.push(child.expression); + ts.forEachChild(child, visit); + }; + ts.forEachChild(node, visit); + + if (rendersJsx || returned.length === 0) return 'component'; + + const isValueLiteral = (expression: ts.Node) => + ts.isObjectLiteralExpression(expression) || + ts.isArrayLiteralExpression(expression) || + ts.isStringLiteralLike(expression) || + ts.isNumericLiteral(expression); + + return returned.every(isValueLiteral) ? 'value' : 'component'; +} + /** * A class is a component only if it extends React's. * @@ -222,11 +309,10 @@ describe('server components take only components from client modules', () => { seen.add(node); if (ts.isFunctionDeclaration(node) || ts.isArrowFunction(node) || ts.isFunctionExpression(node)) { - return 'component'; + return classifyFunction(node); } if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node); - // `styled.div\`…\`` and friends. - if (ts.isTaggedTemplateExpression(node)) return 'component'; + if (ts.isTaggedTemplateExpression(node)) return 'value'; if (ts.isCallExpression(node)) return isComponentWrapper(node.expression) ? 'component' : 'value'; if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { return classify(node.expression, seen); @@ -348,7 +434,7 @@ describe('server components take only components from client modules', () => { const found: Reference[] = []; for (const statement of sourceFile.statements) { - if (ts.isImportDeclaration(statement) && ts.isStringLiteralLike(statement.moduleSpecifier)) { + if (ts.isImportDeclaration(statement) && ts.isStringLiteral(statement.moduleSpecifier)) { const specifier = statement.moduleSpecifier.text; const clause = statement.importClause; @@ -379,7 +465,7 @@ describe('server components take only components from client modules', () => { if ( ts.isExportDeclaration(statement) && statement.moduleSpecifier && - ts.isStringLiteralLike(statement.moduleSpecifier) + ts.isStringLiteral(statement.moduleSpecifier) ) { if (statement.isTypeOnly) continue; const specifier = statement.moduleSpecifier.text; @@ -407,9 +493,7 @@ describe('server components take only components from client modules', () => { } } - for (const [, specifier] of (contentsByFile.get(file) ?? '').matchAll(DYNAMIC_IMPORT)) { - found.push({ specifier, local: '' }); - } + for (const specifier of dynamicImportSpecifiers(sourceFile)) found.push({ specifier, local: '' }); referencesByFile.set(file, found); return found; From a3defcebd1ff8929ec7a6d967f4cc5dfafd00fff Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 14:07:19 -0700 Subject: [PATCH 06/23] test: skip every nested scope, not three kinds of function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classifyFunction` skipped nested function declarations, arrows and function expressions by listing them, and a method or accessor on a nested class is a scope too — so its `return {}` was attributed to the component containing it, which is enough to classify a real component as a value. Planted exactly that and the previous commit reported it. `ts.isFunctionLike` is both correct and shorter. Listing kinds by hand is how most of this file's earlier findings happened. --- .../client-values-in-server-boundaries.test.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 6f1163b419..2ce05334ec 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -228,13 +228,10 @@ function classifyFunction(node: ts.SignatureDeclaration): ExportKind { let rendersJsx = false; const visit = (child: ts.Node) => { - // A nested function's returns are its own, not this one's. - if ( - child !== node && - (ts.isFunctionDeclaration(child) || ts.isArrowFunction(child) || ts.isFunctionExpression(child)) - ) { - return; - } + // Anything with its own body owns its own returns. `isFunctionLike` rather than the three + // function kinds by hand: a method or an accessor on a nested class is a scope too, and + // listing kinds is how the earlier version of this file kept being wrong. + if (child !== node && ts.isFunctionLike(child)) return; if (ts.isJsxElement(child) || ts.isJsxSelfClosingElement(child) || ts.isJsxFragment(child)) rendersJsx = true; if (ts.isReturnStatement(child) && child.expression) returned.push(child.expression); ts.forEachChild(child, visit); From 91513296c40deb832a1aa1c3b87c3e488c1ad8ad Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 14:28:29 -0700 Subject: [PATCH 07/23] test: follow re-exports to where the thing is declared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings. Three implemented, one declined, and the one that mattered was raised as already-missed rather than new. A client barrel re-exporting a component was called unclassifiable, and so reported. That shape is in this tree three times over — `table-block.tsx` re-exports `TableBlockLoadingPlaceholder`, `community-filter-pill.tsx` re-exports `FilterPillTrigger` — so this was a false accusation waiting for somebody to import through a barrel. The chain is followed to the declaring module now, with a cycle guard, and only a complete answer is cached: a run that gave up on a loop must not become the answer every later caller gets. Proved both ways — the component re-export goes quiet, and `FILTER_PILL_CLASS` through the same barrel is still caught. `() => ({})` slipped through the door the block form was closed on last commit, because a concise arrow has no return statement to inspect. A concise body counts as a return now, and a literal is recognised through parentheses, `as`, `satisfies` and `!`. Deciding from the returns also drops the separate JSX scan, which could override a definite literal return — a function handing back `{ label: }` returns an object whatever it renders on the way. `import(`./helper`)` is a legal, statically resolvable specifier, unlike `` `use client` `` as a directive; the dynamic-import reader takes `StringLiteralLike` again while the directive check keeps the stricter one. Declined: judging `{ panel as Panel }` by its local alias. The opposite spelling, `{ Button as button }`, wants the opposite rule, and neither exists here — so the gate stays on the exported name, which is the declaration being classified and the name React would need to render it. The half of that finding which is unambiguous is in: `export { default } from './client'` has no name anyone chose, so capitalisation has nothing to say and the declaration decides alone. --- ...client-values-in-server-boundaries.test.ts | 102 ++++++++++++++---- 1 file changed, 83 insertions(+), 19 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 2ce05334ec..5505fc34c5 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -90,8 +90,10 @@ function dynamicImportSpecifiers(sourceFile: ts.SourceFile): string[] { const visit = (node: ts.Node) => { if (ts.isCallExpression(node) && node.expression.kind === ts.SyntaxKind.ImportKeyword) { const [specifier] = node.arguments; - // A computed specifier resolves to no file, so there is nothing to follow. - if (specifier && ts.isStringLiteral(specifier)) found.push(specifier.text); + // `isStringLiteralLike`, unlike the directive check: `import(`./helper`)` is a legal and + // statically resolvable specifier, where `` `use client` `` is not a legal directive. A + // computed specifier resolves to no file, so there is nothing to follow. + if (specifier && ts.isStringLiteralLike(specifier)) found.push(specifier.text); } ts.forEachChild(node, visit); }; @@ -219,34 +221,56 @@ function isComponentWrapper(expression: ts.Expression): boolean { * * So the evidence runs the other way: a function whose every `return` hands back an object, array, * string or number is a value — which is `function BuildOptions() { return {}; }`, the case review - * named — and anything else is left as a component. That leaves a residue, a function returning a - * value through a variable rather than a literal, and it is the right residue to have: silent where - * it is unsure, loud only where it is certain. + * named — and anything else is left as a component. A concise arrow body counts as a return, or + * `() => ({})` slips through the same door the block form was just closed on, and a literal is + * recognised through parentheses, `as`, `satisfies` and `!`. + * + * Deciding from the returns alone also drops the separate JSX scan this used to run, which could + * override a definite literal return — a function handing back `{ label: }` returns an + * object, whatever it renders on the way. That leaves a residue, a function returning a value + * through a variable rather than a literal, and it is the right residue to have: silent where it is + * unsure, loud only where it is certain. */ function classifyFunction(node: ts.SignatureDeclaration): ExportKind { - const returned: ts.Node[] = []; - let rendersJsx = false; + const returned: ts.Expression[] = []; + + // `() => ({})` has no return statement at all, which is how the first version of this accepted + // the very example it was written to catch — in the other half of the syntax. + if (ts.isArrowFunction(node) && node.body && !ts.isBlock(node.body)) returned.push(node.body); const visit = (child: ts.Node) => { // Anything with its own body owns its own returns. `isFunctionLike` rather than the three // function kinds by hand: a method or an accessor on a nested class is a scope too, and // listing kinds is how the earlier version of this file kept being wrong. if (child !== node && ts.isFunctionLike(child)) return; - if (ts.isJsxElement(child) || ts.isJsxSelfClosingElement(child) || ts.isJsxFragment(child)) rendersJsx = true; if (ts.isReturnStatement(child) && child.expression) returned.push(child.expression); ts.forEachChild(child, visit); }; ts.forEachChild(node, visit); - if (rendersJsx || returned.length === 0) return 'component'; + // Returning nothing is what an effect-only component does. + if (returned.length === 0) return 'component'; - const isValueLiteral = (expression: ts.Node) => + return returned.every(returnsAValue) ? 'value' : 'component'; +} + +/** A literal, through whatever is wrapped around it. */ +function returnsAValue(expression: ts.Expression): boolean { + if ( + ts.isParenthesizedExpression(expression) || + ts.isAsExpression(expression) || + ts.isSatisfiesExpression(expression) || + ts.isNonNullExpression(expression) + ) { + return returnsAValue(expression.expression); + } + + return ( ts.isObjectLiteralExpression(expression) || ts.isArrayLiteralExpression(expression) || ts.isStringLiteralLike(expression) || - ts.isNumericLiteral(expression); - - return returned.every(isValueLiteral) ? 'value' : 'component'; + ts.isNumericLiteral(expression) + ); } /** @@ -280,7 +304,7 @@ describe('server components take only components from client modules', () => { * reaches the arrow function declared above it instead of giving up and reporting a real * component. */ - function kindsFor(file: string): Map { + function kindsFor(file: string, visiting: Set = new Set()): Map { const cached = exportKinds.get(file); if (cached) return cached; @@ -364,20 +388,49 @@ describe('server components take only components from client modules', () => { continue; } - // `export { a }` and `export { a } from '…'`: resolvable only when declared here. if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamedExports(statement.exportClause)) { + /* + * `export { a }` is declared here. `export { a } from './b'` is declared in `./b`, and a + * client barrel doing exactly that is a real shape in this tree — `table-block.tsx` + * re-exports `TableBlockLoadingPlaceholder`, `community-filter-pill.tsx` re-exports + * `FilterPillTrigger`. Calling those unclassifiable accuses real components, so the chain + * is followed to wherever the thing is actually declared. + * + * `visiting` is the cycle guard: barrels re-export each other, and a loop here would be an + * infinite one rather than a wrong answer. + */ + const origin = + statement.moduleSpecifier && ts.isStringLiteral(statement.moduleSpecifier) + ? resolveImport(statement.moduleSpecifier.text, file) + : null; + for (const element of statement.exportClause.elements) { if (statement.isTypeOnly || element.isTypeOnly) { kinds.set(element.name.text, 'erased'); continue; } - const local = locals.get((element.propertyName ?? element.name).text); - kinds.set(element.name.text, local ? classify(local) : 'unknown'); + + const sourceName = (element.propertyName ?? element.name).text; + + if (!origin) { + const local = locals.get(sourceName); + kinds.set(element.name.text, local ? classify(local) : 'unknown'); + continue; + } + + if (visiting.has(origin)) { + kinds.set(element.name.text, 'unknown'); + continue; + } + + kinds.set(element.name.text, kindsFor(origin, new Set([...visiting, file])).get(sourceName) ?? 'unknown'); } } } - exportKinds.set(file, kinds); + // Cached only for a complete answer. A run that gave up on a cycle would otherwise be the + // answer every later caller got. + if (visiting.size === 0) exportKinds.set(file, kinds); return kinds; } @@ -556,8 +609,19 @@ describe('server components take only components from client modules', () => { } if (!reference.exported) continue; + /* + * The gate reads the exported name, because that is the declaration being classified and + * React cannot render a lowercase binding as an element. A default import is judged by its + * local name instead, which is the only name it has. + * + * `export { default } from './client'` has neither — `default` is not a name anyone chose — + * so there is nothing for capitalisation to say and the declaration decides alone. Review + * also asked for `{ panel as Panel }` to be judged by the local alias; that one is declined + * below, in the reply, because the opposite alias `{ Button as button }` wants the opposite + * rule and neither spelling exists here. + */ const named = reference.exported === 'default' ? reference.local : reference.exported; - if (!isCapitalised(named)) { + if (named !== 'default' && !isCapitalised(named)) { yield { offence: `${from} -> ${reference.local} (from ${source})${suffix}`, kind: 'value', From 063fade05c3f917f08b1d4d611f99a6774fbf994 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 14:48:42 -0700 Subject: [PATCH 08/23] test: check the classifier against written-down cases, not only the tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, and the third is the one I should have seen myself. Counting the current tree only ever exercised the *accepting* half of the classifier. Every binding on the allowlist is lowercase or all-uppercase, so it never reaches `exportKindsOf` at all, and a regression that started calling an `Error` subclass or an object-returning function a component would have left all four of those assertions unmoved. That is the same vacuous-assertion problem this branch has already been caught by twice, this time in the assertion written to prevent it. So the classifier is a top-level function taking a source file and a resolver, and there are 28 fixtures over it — ten accepted forms, fourteen rejected ones, types, a followed re-export, an unreadable origin, and a nested scope's return. Each is a case this guard got wrong at some point, which is why the list reads like a changelog. They were all verified by hand at the time, by planting them in the tree and watching the previous version disagree; written down, they stay verified. Five mutations confirm they bite: breaking the class heritage check, the `NewExpression` case, the tagged template, the concise-arrow body and the re-export lookup each fails exactly the fixture for it. The other two. `return -1` is a prefix-unary expression around a literal rather than a literal, and `return new Date()` is a `NewExpression` — the variable classifier already counted both as values while the return classifier let them make a utility look like a component; the halves agree now. And `export * from` is not a namespace value: it hands on the target's named bindings, so a barrel star-re-exporting nothing but components was an unconditional offence. Each binding is classified on its own now, with `export * as Ns` still treated as the namespace object it is. --- ...client-values-in-server-boundaries.test.ts | 396 ++++++++++++------ 1 file changed, 272 insertions(+), 124 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 5505fc34c5..fb2918d449 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -254,7 +254,14 @@ function classifyFunction(node: ts.SignatureDeclaration): ExportKind { return returned.every(returnsAValue) ? 'value' : 'component'; } -/** A literal, through whatever is wrapped around it. */ +/** + * A definite value, through whatever is wrapped around it. + * + * `return -1` is a `PrefixUnaryExpression` around a numeric literal rather than a literal, and + * `return new Date()` is a `NewExpression` — both of which the variable-initialiser classifier + * already treated as values while this one let them make a utility look like a component. The two + * halves agree now. + */ function returnsAValue(expression: ts.Expression): boolean { if ( ts.isParenthesizedExpression(expression) || @@ -265,11 +272,24 @@ function returnsAValue(expression: ts.Expression): boolean { return returnsAValue(expression.expression); } + // `-1`, `+1`, and `!0` — a sign or a negation around a literal is still a literal. + if (ts.isPrefixUnaryExpression(expression)) { + const signs: ts.PrefixUnaryOperator[] = [ + ts.SyntaxKind.MinusToken, + ts.SyntaxKind.PlusToken, + ts.SyntaxKind.ExclamationToken, + ]; + return signs.includes(expression.operator) && returnsAValue(expression.operand); + } + return ( ts.isObjectLiteralExpression(expression) || ts.isArrayLiteralExpression(expression) || ts.isStringLiteralLike(expression) || - ts.isNumericLiteral(expression) + ts.isNumericLiteral(expression) || + ts.isNewExpression(expression) || + expression.kind === ts.SyntaxKind.TrueKeyword || + expression.kind === ts.SyntaxKind.FalseKeyword ); } @@ -288,148 +308,168 @@ function classifyClass(node: ts.ClassLikeDeclaration): ExportKind { return extended.some(name => /(^|\.)(Pure)?Component$/.test(name)) ? 'component' : 'value'; } -describe('server components take only components from client modules', () => { - const files = sourceFiles(); - const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); - const astByFile = new Map([...contentsByFile].map(([file, contents]) => [file, parse(file, contents)])); - const clientFiles = new Set([...astByFile].filter(([, ast]) => isClientModule(ast)).map(([file]) => file)); +/** + * What each of a module's exports is, by exported name, with `default` keyed as `default`. + * + * Top-level rather than closed over the file map so it can be handed a source string and checked + * directly — see the fixtures at the bottom of this file. Review's point was that counting the + * current tree exercises only the *accepting* half of this function: every binding on the allowlist + * is lowercase or all-uppercase, so it never reaches here, and a regression that started calling an + * `Error` subclass a component would leave every count unchanged. + * + * `resolveOrigin` answers what a re-export's target exports, or null where there is nothing to + * follow. + * + * An initialiser is followed where following it answers the question: through parentheses, `as` and + * `satisfies`, and through a local identifier — which is how `export default SuggestedFormats` + * reaches the arrow function declared above it instead of giving up and reporting a real component. + */ +function exportKindsOf( + sourceFile: ts.SourceFile, + resolveOrigin: (specifier: string) => Map | null +): Map { + const kinds = new Map(); + /** Local declarations, so an export by identifier has something to resolve against. */ + const locals = new Map(); - const exportKinds = new Map>(); + for (const statement of sourceFile.statements) { + if (ts.isVariableStatement(statement)) { + for (const declaration of statement.declarationList.declarations) { + if (ts.isIdentifier(declaration.name) && declaration.initializer) { + locals.set(declaration.name.text, declaration.initializer); + } + } + } else if ((ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) && statement.name) { + locals.set(statement.name.text, statement); + } + } - /** - * What each of a module's exports is, by exported name, with `default` keyed as `default`. - * - * An initialiser is followed where following it answers the question: through parentheses, `as` - * and `satisfies`, and through a local identifier — which is how `export default SuggestedFormats` - * reaches the arrow function declared above it instead of giving up and reporting a real - * component. - */ - function kindsFor(file: string, visiting: Set = new Set()): Map { - const cached = exportKinds.get(file); - if (cached) return cached; + const classify = (node: ts.Node, seen = new Set()): ExportKind => { + if (seen.has(node)) return 'unknown'; + seen.add(node); - const sourceFile = astByFile.get(file)!; - const kinds = new Map(); - /** Local declarations, so an export by identifier has something to resolve against. */ - const locals = new Map(); + if (ts.isFunctionDeclaration(node) || ts.isArrowFunction(node) || ts.isFunctionExpression(node)) { + return classifyFunction(node); + } + if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node); + if (ts.isTaggedTemplateExpression(node)) return 'value'; + if (ts.isCallExpression(node)) return isComponentWrapper(node.expression) ? 'component' : 'value'; + if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { + return classify(node.expression, seen); + } + if (ts.isIdentifier(node)) { + const local = locals.get(node.text); + return local ? classify(local, seen) : 'unknown'; + } + if ( + ts.isObjectLiteralExpression(node) || + ts.isArrayLiteralExpression(node) || + ts.isStringLiteralLike(node) || + ts.isNumericLiteral(node) || + ts.isNewExpression(node) || + node.kind === ts.SyntaxKind.TrueKeyword || + node.kind === ts.SyntaxKind.FalseKeyword + ) { + return 'value'; + } + return 'unknown'; + }; - for (const statement of sourceFile.statements) { - if (ts.isVariableStatement(statement)) { - for (const declaration of statement.declarationList.declarations) { - if (ts.isIdentifier(declaration.name) && declaration.initializer) { - locals.set(declaration.name.text, declaration.initializer); - } - } - } else if ((ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) && statement.name) { - locals.set(statement.name.text, statement); - } + for (const statement of sourceFile.statements) { + if (ts.isTypeAliasDeclaration(statement) || ts.isInterfaceDeclaration(statement)) { + if (isExported(statement)) kinds.set(statement.name.text, 'erased'); + continue; } - const classify = (node: ts.Node, seen = new Set()): ExportKind => { - if (seen.has(node)) return 'unknown'; - seen.add(node); + // An enum is an object at runtime, whatever it looks like in the types. + if (ts.isEnumDeclaration(statement) && isExported(statement)) { + kinds.set(statement.name.text, 'value'); + continue; + } - if (ts.isFunctionDeclaration(node) || ts.isArrowFunction(node) || ts.isFunctionExpression(node)) { - return classifyFunction(node); - } - if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node); - if (ts.isTaggedTemplateExpression(node)) return 'value'; - if (ts.isCallExpression(node)) return isComponentWrapper(node.expression) ? 'component' : 'value'; - if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { - return classify(node.expression, seen); - } - if (ts.isIdentifier(node)) { - const local = locals.get(node.text); - return local ? classify(local, seen) : 'unknown'; - } - if ( - ts.isObjectLiteralExpression(node) || - ts.isArrayLiteralExpression(node) || - ts.isStringLiteralLike(node) || - ts.isNumericLiteral(node) || - ts.isNewExpression(node) || - node.kind === ts.SyntaxKind.TrueKeyword || - node.kind === ts.SyntaxKind.FalseKeyword - ) { - return 'value'; - } - return 'unknown'; - }; + if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) { + if (!isExported(statement)) continue; + kinds.set(isDefault(statement) ? 'default' : (statement.name?.text ?? 'default'), classify(statement)); + continue; + } - for (const statement of sourceFile.statements) { - if (ts.isTypeAliasDeclaration(statement) || ts.isInterfaceDeclaration(statement)) { - if (isExported(statement)) kinds.set(statement.name.text, 'erased'); - continue; + if (ts.isVariableStatement(statement) && isExported(statement)) { + for (const declaration of statement.declarationList.declarations) { + if (!ts.isIdentifier(declaration.name)) continue; + kinds.set(declaration.name.text, declaration.initializer ? classify(declaration.initializer) : 'unknown'); } + continue; + } - // An enum is an object at runtime, whatever it looks like in the types. - if (ts.isEnumDeclaration(statement) && isExported(statement)) { - kinds.set(statement.name.text, 'value'); - continue; - } + // `export default `, a bare identifier included. + if (ts.isExportAssignment(statement) && !statement.isExportEquals) { + kinds.set('default', classify(statement.expression)); + continue; + } - if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) { - if (!isExported(statement)) continue; - kinds.set(isDefault(statement) ? 'default' : (statement.name?.text ?? 'default'), classify(statement)); - continue; - } + if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamedExports(statement.exportClause)) { + /* + * `export { a }` is declared here. `export { a } from './b'` is declared in `./b`, and a + * client barrel doing exactly that is a real shape in this tree — `table-block.tsx` + * re-exports `TableBlockLoadingPlaceholder`, `community-filter-pill.tsx` re-exports + * `FilterPillTrigger`. Calling those unclassifiable accuses real components, so the chain + * is followed to wherever the thing is actually declared. + * + * `visiting` is the cycle guard: barrels re-export each other, and a loop here would be an + * infinite one rather than a wrong answer. + */ + const origin = + statement.moduleSpecifier && ts.isStringLiteral(statement.moduleSpecifier) + ? resolveOrigin(statement.moduleSpecifier.text) + : null; + + for (const element of statement.exportClause.elements) { + if (statement.isTypeOnly || element.isTypeOnly) { + kinds.set(element.name.text, 'erased'); + continue; + } + + const sourceName = (element.propertyName ?? element.name).text; - if (ts.isVariableStatement(statement) && isExported(statement)) { - for (const declaration of statement.declarationList.declarations) { - if (!ts.isIdentifier(declaration.name)) continue; - kinds.set(declaration.name.text, declaration.initializer ? classify(declaration.initializer) : 'unknown'); + if (!origin) { + const local = locals.get(sourceName); + kinds.set(element.name.text, local ? classify(local) : 'unknown'); + continue; } - continue; - } - // `export default `, a bare identifier included. - if (ts.isExportAssignment(statement) && !statement.isExportEquals) { - kinds.set('default', classify(statement.expression)); - continue; + kinds.set(element.name.text, origin.get(sourceName) ?? 'unknown'); } + } + } - if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamedExports(statement.exportClause)) { - /* - * `export { a }` is declared here. `export { a } from './b'` is declared in `./b`, and a - * client barrel doing exactly that is a real shape in this tree — `table-block.tsx` - * re-exports `TableBlockLoadingPlaceholder`, `community-filter-pill.tsx` re-exports - * `FilterPillTrigger`. Calling those unclassifiable accuses real components, so the chain - * is followed to wherever the thing is actually declared. - * - * `visiting` is the cycle guard: barrels re-export each other, and a loop here would be an - * infinite one rather than a wrong answer. - */ - const origin = - statement.moduleSpecifier && ts.isStringLiteral(statement.moduleSpecifier) - ? resolveImport(statement.moduleSpecifier.text, file) - : null; - - for (const element of statement.exportClause.elements) { - if (statement.isTypeOnly || element.isTypeOnly) { - kinds.set(element.name.text, 'erased'); - continue; - } + return kinds; +} - const sourceName = (element.propertyName ?? element.name).text; +describe('server components take only components from client modules', () => { + const files = sourceFiles(); + const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); + const astByFile = new Map([...contentsByFile].map(([file, contents]) => [file, parse(file, contents)])); + const clientFiles = new Set([...astByFile].filter(([, ast]) => isClientModule(ast)).map(([file]) => file)); - if (!origin) { - const local = locals.get(sourceName); - kinds.set(element.name.text, local ? classify(local) : 'unknown'); - continue; - } + const exportKinds = new Map>(); - if (visiting.has(origin)) { - kinds.set(element.name.text, 'unknown'); - continue; - } + /** + * {@link exportKindsOf} for a file in the tree, memoised, following re-exports across modules. + * + * `visiting` is the cycle guard: barrels re-export each other, and a loop there would be an + * infinite one rather than a wrong answer. Only a complete answer is cached, so a run that gave + * up on a cycle cannot become the answer every later caller gets. + */ + function kindsFor(file: string, visiting: Set = new Set()): Map { + const cached = exportKinds.get(file); + if (cached) return cached; - kinds.set(element.name.text, kindsFor(origin, new Set([...visiting, file])).get(sourceName) ?? 'unknown'); - } - } - } + const kinds = exportKindsOf(astByFile.get(file)!, specifier => { + const origin = resolveImport(specifier, file); + if (!origin || visiting.has(origin)) return null; + return kindsFor(origin, new Set([...visiting, file])); + }); - // Cached only for a complete answer. A run that gave up on a cycle would otherwise be the - // answer every later caller got. if (visiting.size === 0) exportKinds.set(file, kinds); return kinds; } @@ -462,7 +502,10 @@ describe('server components take only components from client modules', () => { specifier: string; exported?: string; local: string; + /** `import * as X` / `export * as X`: an object whose every property is a client reference. */ namespace?: boolean; + /** `export * from`: the target's named bindings, each classified on its own. */ + starReexport?: boolean; reexported?: boolean; }; @@ -521,7 +564,10 @@ describe('server components take only components from client modules', () => { const specifier = statement.moduleSpecifier.text; if (!statement.exportClause) { - found.push({ specifier, local: 're-exports *', namespace: true, reexported: true }); + // Not a namespace value: this hands on the target's named bindings one by one, so a + // barrel star-re-exporting nothing but components is not an offence. `export * as Ns` is + // a namespace object and stays one, below. + found.push({ specifier, local: 're-exports *', starReexport: true, reexported: true }); } else if (ts.isNamespaceExport(statement.exportClause)) { found.push({ specifier, @@ -607,6 +653,25 @@ describe('server components take only components from client modules', () => { yield { offence: `${from} -> ${reference.local} (from ${source})`, kind: 'value', capitalised: false }; continue; } + + // `export * from` is every named binding the target has, so each is judged on its own. + if (reference.starReexport) { + for (const [exported, kind] of kindsFor(target)) { + if (kind === 'component' || kind === 'erased') continue; + if (isCapitalised(exported)) { + const label = kind === 'unknown' ? 'unclassifiable ' : ''; + yield { offence: `${from} -> re-exports ${exported} (${label}from ${source})`, kind, capitalised: true }; + } else { + yield { + offence: `${from} -> re-exports ${exported} (from ${source})`, + kind: 'value', + capitalised: false, + }; + } + } + continue; + } + if (!reference.exported) continue; /* @@ -686,3 +751,86 @@ describe('server components take only components from client modules', () => { expect(kinds.unknown).toBe(0); }); }); + +/** + * The classifier against written-down cases, not only against the tree. + * + * Review's objection to the corpus count was exact: every binding on the allowlist is lowercase or + * all-uppercase, so it never reaches `exportKindsOf`, and the count therefore exercised only the + * accepting half. A regression that started calling an `Error` subclass or an object-returning + * function a component would have left all four of those assertions unmoved. + * + * Each case below is one this guard got wrong at some point, which is why the list reads like a + * changelog. They were verified by hand at the time, by planting them in the tree and watching the + * previous version disagree; written down here, they stay verified. + */ +describe('exportKindsOf', () => { + const kindOf = (source: string, exported = 'Subject') => + exportKindsOf(parse('fixture.tsx', source), () => null).get(exported); + + it.each([ + ['a function declaration', 'export function Subject() { return
; }'], + ['a function that renders nothing but runs effects', 'export function Subject() { return null; }'], + ['a function with no return at all', 'export function Subject() { useThing(); }'], + ['an arrow', 'export const Subject = () =>
;'], + ['memo', 'export const Subject = memo(() =>
);'], + ['React.forwardRef', 'export const Subject = React.forwardRef(() =>
);'], + ['a class extending Component', 'export class Subject extends React.Component {}'], + ['an identifier default', 'const Inner = () =>
;\nexport default Inner;'], + ['a parenthesised arrow', 'export const Subject = ((props) =>
);'], + ['an arrow returning a component call', 'export const Subject = () => renderThing();'], + ])('accepts %s', (_label, source) => { + expect(kindOf(source, source.includes('export default') ? 'default' : 'Subject')).toBe('component'); + }); + + it.each([ + ['an object', 'export const Subject = { a: 1 };'], + ['a parenthesised object', 'export const Subject = ({ a: 1 });'], + ['an array', 'export const Subject = [1, 2];'], + ['a string', "export const Subject = 'x';"], + ['a call that is not memo or forwardRef', "export const Subject = cva('x');"], + ['a tagged template', 'export const Subject = gql`query { x }`;'], + ['a class extending Error', 'export class Subject extends Error {}'], + ['an enum', 'export enum Subject { A }'], + ['a function returning an object', 'export function Subject() { return {}; }'], + ['a concise arrow returning an object', 'export const Subject = () => ({});'], + ['a concise arrow returning an asserted object', 'export const Subject = () => ({}) as Thing;'], + ['a function returning a negative number', 'export function Subject() { return -1; }'], + ['a function returning a constructed object', 'export function Subject() { return new Date(); }'], + ['a default object', 'export default { a: 1 };'], + ])('rejects %s', (_label, source) => { + expect(kindOf(source, source.includes('export default') ? 'default' : 'Subject')).toBe('value'); + }); + + it('treats a type as erased, however it is written', () => { + expect(kindOf('export type Subject = { a: 1 };')).toBe('erased'); + expect(kindOf('export interface Subject { a: 1 }')).toBe('erased'); + expect(kindOf("export type { Subject } from './x';")).toBe('erased'); + }); + + it('follows a re-export to whatever the origin says it is', () => { + const origin = new Map([ + ['Button', 'component'], + ['BUTTON_CLASS', 'value'], + ]); + const kinds = exportKindsOf(parse('barrel.tsx', "export { Button, BUTTON_CLASS } from './button';"), () => origin); + + expect(kinds.get('Button')).toBe('component'); + expect(kinds.get('BUTTON_CLASS')).toBe('value'); + }); + + it('gives up rather than guessing when the origin cannot be read', () => { + // What a cycle guard hands back, and what an unresolvable specifier does. Reported, not waved + // through: `unknown` is an offence, so the failure is visible. + expect(kindOf("export { Subject } from './somewhere-unreadable';")).toBe('unknown'); + }); + + it("does not attribute a nested scope's return to the component containing it", () => { + // A method on a nested class is a scope of its own; its `return {}` is not the component's. + expect( + kindOf( + 'export function Subject() {\n class Helper {\n build() {\n return {};\n }\n }\n void new Helper();\n}' + ) + ).toBe('component'); + }); +}); From 9e0c19fe90b254b4c93b68ab5a70229fd359038b Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 15:52:54 -0700 Subject: [PATCH 09/23] test: classify by what React can render, and resolve imported bindings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, four taken, one half-taken — and one of them reverses a change made at review's own request two rounds ago. What a function returns is now judged by whether React could render it, not by whether it is a literal. `function Badge() { return 'New'; }` is a real component, and so is one returning a number or an array of elements; React renders all three and renders nothing at all for a boolean. Calling those values accused real components, which is the failure this guard cannot afford. What React genuinely cannot render — a plain object, a regular expression, anything from `new` — is what stays. That drops the signed-numeric case added last round: `return -1` renders as "-1", so it is a component, and the fixture moves from rejected to accepted. `export const Subject = 'x'` is still a value, because what an export *is* and what a function *returns* are different questions; there is a fixture pinning the pair. `memo` and `forwardRef` now have to be React's. Matching the callee's name alone took `helpers.memo(config)` or a locally declared `forwardRef` for the real thing. All nine call sites in this tree are React's, so this is prevention. Identifiers are resolved through imports as well as local declarations, in both places that ask: `import { Button } from './button'; export { Button };` is a barrel that declares nothing, and calling its component unclassifiable reported it. One module here is written that way. The fixture for it caught that my first fix only covered one of the two paths. `export * from` is read in both directions now — as a source of named exports when a client barrel is written that way, and without a `default`, which `export *` does not carry and which the checker was inventing offences from. Half declined, with the reason in the reply: an all-type-specifier import is not a runtime edge here. `verbatimModuleSyntax` is off in this tsconfig, so TypeScript elides it, and adding phantom edges is how this guard produced false offences two rounds ago. The other half is in — `import {} from './x'` has a clause with nothing in it and does load the module. 41 fixtures. Four mutations, four distinct failures. --- ...client-values-in-server-boundaries.test.ts | 200 +++++++++++++++--- 1 file changed, 170 insertions(+), 30 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index fb2918d449..1399610979 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -201,9 +201,48 @@ type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; /** `memo(X)` and `forwardRef(X)`, plain or `React.`-qualified, produce components. Nothing else does. */ const COMPONENT_WRAPPERS = new Set(['memo', 'forwardRef']); -function isComponentWrapper(expression: ts.Expression): boolean { - if (ts.isIdentifier(expression)) return COMPONENT_WRAPPERS.has(expression.text); - if (ts.isPropertyAccessExpression(expression)) return COMPONENT_WRAPPERS.has(expression.name.text); +/** The local names in one module that actually refer to React's `memo` and `forwardRef`. */ +type ReactBindings = { wrappers: Set; namespaces: Set }; + +/** + * Which spellings of `memo` and `forwardRef` this file has earned. + * + * Matching the callee's name alone would take `helpers.memo(config)` or a locally declared + * `forwardRef` for React's, and hand a component exemption to an ordinary value. Neither spelling + * exists in this tree — all nine call sites are React's — so this is prevention, and cheap because + * the imports are already parsed. + */ +function reactBindingsOf(sourceFile: ts.SourceFile): ReactBindings { + const wrappers = new Set(); + const namespaces = new Set(); + + for (const statement of sourceFile.statements) { + if (!ts.isImportDeclaration(statement) || !ts.isStringLiteral(statement.moduleSpecifier)) continue; + if (statement.moduleSpecifier.text !== 'react') continue; + + const clause = statement.importClause; + if (!clause || clause.isTypeOnly) continue; + + if (clause.name) namespaces.add(clause.name.text); + + if (clause.namedBindings && ts.isNamespaceImport(clause.namedBindings)) { + namespaces.add(clause.namedBindings.name.text); + } else if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { + for (const element of clause.namedBindings.elements) { + if (element.isTypeOnly) continue; + if (COMPONENT_WRAPPERS.has((element.propertyName ?? element.name).text)) wrappers.add(element.name.text); + } + } + } + + return { wrappers, namespaces }; +} + +function isComponentWrapper(expression: ts.Expression, react: ReactBindings): boolean { + if (ts.isIdentifier(expression)) return react.wrappers.has(expression.text); + if (ts.isPropertyAccessExpression(expression) && ts.isIdentifier(expression.expression)) { + return react.namespaces.has(expression.expression.text) && COMPONENT_WRAPPERS.has(expression.name.text); + } return false; } @@ -255,12 +294,19 @@ function classifyFunction(node: ts.SignatureDeclaration): ExportKind { } /** - * A definite value, through whatever is wrapped around it. + * A return React could not render, through whatever is wrapped around it. + * + * Narrower than "a literal", and deliberately so. `function Badge() { return 'New'; }` is a real + * component, and so is one returning a number or an array of elements; React renders all three, and + * renders nothing at all for a boolean. Calling those values would accuse real components — the + * failure this guard cannot afford. What React genuinely cannot render is a plain object, a regular + * expression, or anything from `new`, and those stay. + * + * This reverses the `return -1` half of an earlier round, which asked for signed numerics to count + * as values. They are renderable, so they do not. * - * `return -1` is a `PrefixUnaryExpression` around a numeric literal rather than a literal, and - * `return new Date()` is a `NewExpression` — both of which the variable-initialiser classifier - * already treated as values while this one let them make a utility look like a component. The two - * halves agree now. + * Note this is only about what a function *returns*. `export const Subject = 'x'` is still a string + * rather than a component — a different question, answered by the initialiser classifier. */ function returnsAValue(expression: ts.Expression): boolean { if ( @@ -272,24 +318,10 @@ function returnsAValue(expression: ts.Expression): boolean { return returnsAValue(expression.expression); } - // `-1`, `+1`, and `!0` — a sign or a negation around a literal is still a literal. - if (ts.isPrefixUnaryExpression(expression)) { - const signs: ts.PrefixUnaryOperator[] = [ - ts.SyntaxKind.MinusToken, - ts.SyntaxKind.PlusToken, - ts.SyntaxKind.ExclamationToken, - ]; - return signs.includes(expression.operator) && returnsAValue(expression.operand); - } - return ( ts.isObjectLiteralExpression(expression) || - ts.isArrayLiteralExpression(expression) || - ts.isStringLiteralLike(expression) || - ts.isNumericLiteral(expression) || - ts.isNewExpression(expression) || - expression.kind === ts.SyntaxKind.TrueKeyword || - expression.kind === ts.SyntaxKind.FalseKeyword + ts.isRegularExpressionLiteral(expression) || + ts.isNewExpression(expression) ); } @@ -329,8 +361,16 @@ function exportKindsOf( resolveOrigin: (specifier: string) => Map | null ): Map { const kinds = new Map(); + const react = reactBindingsOf(sourceFile); /** Local declarations, so an export by identifier has something to resolve against. */ const locals = new Map(); + /** + * Imported names, so an identifier that is not declared here still resolves. + * + * `import { Button } from './button'; export { Button };` is a barrel that declares nothing, and + * calling its component unclassifiable reports it. One module in this tree is written that way. + */ + const imported = new Map(); for (const statement of sourceFile.statements) { if (ts.isVariableStatement(statement)) { @@ -341,6 +381,18 @@ function exportKindsOf( } } else if ((ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) && statement.name) { locals.set(statement.name.text, statement); + } else if (ts.isImportDeclaration(statement) && ts.isStringLiteral(statement.moduleSpecifier)) { + const clause = statement.importClause; + if (!clause || clause.isTypeOnly) continue; + const specifier = statement.moduleSpecifier.text; + + if (clause.name) imported.set(clause.name.text, { specifier, exported: 'default' }); + if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { + for (const element of clause.namedBindings.elements) { + if (element.isTypeOnly) continue; + imported.set(element.name.text, { specifier, exported: (element.propertyName ?? element.name).text }); + } + } } } @@ -353,13 +405,17 @@ function exportKindsOf( } if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node); if (ts.isTaggedTemplateExpression(node)) return 'value'; - if (ts.isCallExpression(node)) return isComponentWrapper(node.expression) ? 'component' : 'value'; + if (ts.isCallExpression(node)) return isComponentWrapper(node.expression, react) ? 'component' : 'value'; if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { return classify(node.expression, seen); } if (ts.isIdentifier(node)) { const local = locals.get(node.text); - return local ? classify(local, seen) : 'unknown'; + if (local) return classify(local, seen); + + const source = imported.get(node.text); + if (!source) return 'unknown'; + return resolveOrigin(source.specifier)?.get(source.exported) ?? 'unknown'; } if ( ts.isObjectLiteralExpression(node) || @@ -407,6 +463,23 @@ function exportKindsOf( continue; } + /* + * `export * from './x'` hands on every named export of `./x` — but never its default, which is + * the one binding `export *` does not carry. Without this a client barrel written that way had + * no exports at all as far as this was concerned, so everything taken from it came back + * unclassifiable. + */ + if (ts.isExportDeclaration(statement) && !statement.exportClause && statement.moduleSpecifier) { + if (statement.isTypeOnly || !ts.isStringLiteral(statement.moduleSpecifier)) continue; + const origin = resolveOrigin(statement.moduleSpecifier.text); + if (!origin) continue; + + for (const [name, kind] of origin) { + if (name !== 'default') kinds.set(name, kind); + } + continue; + } + if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamedExports(statement.exportClause)) { /* * `export { a }` is declared here. `export { a } from './b'` is declared in `./b`, and a @@ -433,7 +506,18 @@ function exportKindsOf( if (!origin) { const local = locals.get(sourceName); - kinds.set(element.name.text, local ? classify(local) : 'unknown'); + if (local) { + kinds.set(element.name.text, classify(local)); + continue; + } + + // Declared in neither this statement nor this file: `import { Button } from './button'; + // export { Button };` is a barrel whose export is somebody else's declaration. + const fromImport = imported.get(sourceName); + kinds.set( + element.name.text, + fromImport ? (resolveOrigin(fromImport.specifier)?.get(fromImport.exported) ?? 'unknown') : 'unknown' + ); continue; } @@ -538,6 +622,19 @@ describe('server components take only components from client modules', () => { } if (clause.isTypeOnly) continue; + // `import {} from './x'` has a clause with nothing in it and still loads the module. Review + // also asked for the all-type-specifier form; that is declined in the reply, because this + // repo does not set `verbatimModuleSyntax` and TypeScript elides it. + if ( + !clause.name && + clause.namedBindings && + ts.isNamedImports(clause.namedBindings) && + clause.namedBindings.elements.length === 0 + ) { + found.push({ specifier, local: '' }); + continue; + } + if (clause.name) found.push({ specifier, exported: 'default', local: clause.name.text }); if (clause.namedBindings && ts.isNamespaceImport(clause.namedBindings)) { @@ -657,6 +754,8 @@ describe('server components take only components from client modules', () => { // `export * from` is every named binding the target has, so each is judged on its own. if (reference.starReexport) { for (const [exported, kind] of kindsFor(target)) { + // `export *` does not carry the default export, so reading one here invents an offence. + if (exported === 'default') continue; if (kind === 'component' || kind === 'erased') continue; if (isCapitalised(exported)) { const label = kind === 'unknown' ? 'unclassifiable ' : ''; @@ -773,12 +872,17 @@ describe('exportKindsOf', () => { ['a function that renders nothing but runs effects', 'export function Subject() { return null; }'], ['a function with no return at all', 'export function Subject() { useThing(); }'], ['an arrow', 'export const Subject = () =>
;'], - ['memo', 'export const Subject = memo(() =>
);'], - ['React.forwardRef', 'export const Subject = React.forwardRef(() =>
);'], + ['memo imported from react', "import { memo } from 'react';\nexport const Subject = memo(() =>
);"], + ['React.forwardRef', "import * as React from 'react';\nexport const Subject = React.forwardRef(() =>
);"], ['a class extending Component', 'export class Subject extends React.Component {}'], ['an identifier default', 'const Inner = () =>
;\nexport default Inner;'], ['a parenthesised arrow', 'export const Subject = ((props) =>
);'], ['an arrow returning a component call', 'export const Subject = () => renderThing();'], + // React renders all of these, so a function returning one is a component. + ['a function returning a string', "export function Subject() { return 'New'; }"], + ['a function returning a negative number', 'export function Subject() { return -1; }'], + ['a function returning an array', 'export function Subject() { return []; }'], + ['a function returning false', 'export function Subject() { return false; }'], ])('accepts %s', (_label, source) => { expect(kindOf(source, source.includes('export default') ? 'default' : 'Subject')).toBe('component'); }); @@ -795,13 +899,49 @@ describe('exportKindsOf', () => { ['a function returning an object', 'export function Subject() { return {}; }'], ['a concise arrow returning an object', 'export const Subject = () => ({});'], ['a concise arrow returning an asserted object', 'export const Subject = () => ({}) as Thing;'], - ['a function returning a negative number', 'export function Subject() { return -1; }'], ['a function returning a constructed object', 'export function Subject() { return new Date(); }'], + ['a function returning a regular expression', 'export function Subject() { return /x/; }'], + [ + "a wrapper call that is not React's", + "import { memo } from 'other';\nexport const Subject = memo(() =>
);", + ], ['a default object', 'export default { a: 1 };'], ])('rejects %s', (_label, source) => { expect(kindOf(source, source.includes('export default') ? 'default' : 'Subject')).toBe('value'); }); + it('tells a returned string from an exported one', () => { + // `export const Subject = 'x'` is a string. `function Subject() { return 'x' }` is a component + // that renders one. The same literal, two different questions. + expect(kindOf("export const Subject = 'x';")).toBe('value'); + expect(kindOf("export function Subject() { return 'x'; }")).toBe('component'); + }); + + it('resolves an identifier that was imported rather than declared', () => { + // A barrel that declares nothing: `import { Button } from './button'; export { Button };` + const origin = new Map([['Button', 'component']]); + const kinds = exportKindsOf( + parse('barrel.tsx', "import { Button } from './button';\nexport { Button };"), + () => origin + ); + + expect(kinds.get('Button')).toBe('component'); + }); + + it("carries a star re-export's named exports but not its default", () => { + const origin = new Map([ + ['Button', 'component'], + ['BUTTON_CLASS', 'value'], + ['default', 'value'], + ]); + const kinds = exportKindsOf(parse('barrel.tsx', "export * from './button';"), () => origin); + + expect(kinds.get('Button')).toBe('component'); + expect(kinds.get('BUTTON_CLASS')).toBe('value'); + // `export *` does not carry a default, so claiming one here would invent an offence. + expect(kinds.has('default')).toBe(false); + }); + it('treats a type as erased, however it is written', () => { expect(kindOf('export type Subject = { a: 1 };')).toBe('erased'); expect(kindOf('export interface Subject { a: 1 }')).toBe('erased'); From 71dc7bd39b0819e05ad873a7e2850d8ab8b12b39 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 16:19:20 -0700 Subject: [PATCH 10/23] test: finish the binding check, and rule out what cannot be a component MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all sound, all dormant in this tree — and one of them is my own unfinished work rather than a new idea. Last round I stopped `memo` and `forwardRef` being matched by name and made them prove they came from React. `classifyClass` was the same check on the same kind of mistake and I left it spelling-matched, so an unrelated local `Component` still passed as React's base and React's own base imported under an alias was rejected. The heritage is checked by binding now, which is what "fix the class, not the line" should have meant the first time. Async functions and generators are ruled out before the return heuristic runs at all. A client component hands back an element; an async function hands back a promise and a generator hands back an iterator, and a module that says `use client` has opted out of being the one kind of component allowed to be async. Two smaller ones. `export default interface Foo {}` was keyed under `Foo` while a default import looks it up as `default`, so an erased type came back unclassifiable and got reported. And `export * as Ns from` was recorded nowhere, so a barrel over a barrel lost the binding — it is a value now, because every property read off a client namespace is a client reference. Six new fixtures, 47 in all. Four mutations, and each fails only its own: the async trio, the aliased-base pair, the default interface, the namespace export. --- ...client-values-in-server-boundaries.test.ts | 85 ++++++++++++++++--- 1 file changed, 72 insertions(+), 13 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 1399610979..51d63ab273 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -201,8 +201,11 @@ type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; /** `memo(X)` and `forwardRef(X)`, plain or `React.`-qualified, produce components. Nothing else does. */ const COMPONENT_WRAPPERS = new Set(['memo', 'forwardRef']); -/** The local names in one module that actually refer to React's `memo` and `forwardRef`. */ -type ReactBindings = { wrappers: Set; namespaces: Set }; +/** The local names in one module that actually refer to React's wrappers and base classes. */ +type ReactBindings = { wrappers: Set; bases: Set; namespaces: Set }; + +/** The base classes a React class component may extend. */ +const COMPONENT_BASES = new Set(['Component', 'PureComponent']); /** * Which spellings of `memo` and `forwardRef` this file has earned. @@ -214,6 +217,7 @@ type ReactBindings = { wrappers: Set; namespaces: Set }; */ function reactBindingsOf(sourceFile: ts.SourceFile): ReactBindings { const wrappers = new Set(); + const bases = new Set(); const namespaces = new Set(); for (const statement of sourceFile.statements) { @@ -230,12 +234,14 @@ function reactBindingsOf(sourceFile: ts.SourceFile): ReactBindings { } else if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { for (const element of clause.namedBindings.elements) { if (element.isTypeOnly) continue; - if (COMPONENT_WRAPPERS.has((element.propertyName ?? element.name).text)) wrappers.add(element.name.text); + const exported = (element.propertyName ?? element.name).text; + if (COMPONENT_WRAPPERS.has(exported)) wrappers.add(element.name.text); + if (COMPONENT_BASES.has(exported)) bases.add(element.name.text); } } } - return { wrappers, namespaces }; + return { wrappers, bases, namespaces }; } function isComponentWrapper(expression: ts.Expression, react: ReactBindings): boolean { @@ -326,18 +332,28 @@ function returnsAValue(expression: ts.Expression): boolean { } /** - * A class is a component only if it extends React's. + * A class is a component only if it extends React's, by binding rather than by spelling. * * `export class GeoChatRequestError extends Error` lives in a `'use client'` module and a capital - * letter alone let it through. An Error subclass read on the server is a client reference like any - * other value. + * letter alone let it through; an Error subclass read on the server is a client reference like any + * other value. Matching the *name* `Component` was the next version of the same mistake — it takes + * an unrelated local `Component` for React's, and rejects React's own base imported under an alias. + * This is the wrapper check's twin and should have been fixed in the same commit as it. */ -function classifyClass(node: ts.ClassLikeDeclaration): ExportKind { +function classifyClass(node: ts.ClassLikeDeclaration, react: ReactBindings): ExportKind { const extended = (node.heritageClauses ?? []) .filter(clause => clause.token === ts.SyntaxKind.ExtendsKeyword) - .flatMap(clause => clause.types.map(type => type.expression.getText())); + .flatMap(clause => clause.types.map(type => type.expression)); - return extended.some(name => /(^|\.)(Pure)?Component$/.test(name)) ? 'component' : 'value'; + const isReactBase = (expression: ts.Expression) => { + if (ts.isIdentifier(expression)) return react.bases.has(expression.text); + if (ts.isPropertyAccessExpression(expression) && ts.isIdentifier(expression.expression)) { + return react.namespaces.has(expression.expression.text) && COMPONENT_BASES.has(expression.name.text); + } + return false; + }; + + return extended.some(isReactBase) ? 'component' : 'value'; } /** @@ -401,9 +417,16 @@ function exportKindsOf( seen.add(node); if (ts.isFunctionDeclaration(node) || ts.isArrowFunction(node) || ts.isFunctionExpression(node)) { + // A client component is neither of these: an async function hands back a promise and a + // generator hands back an iterator. Only a Server Component may be async, and a module that + // says `use client` has opted out of being one. + const isAsync = (ts.getModifiers(node) ?? []).some(modifier => modifier.kind === ts.SyntaxKind.AsyncKeyword); + const isGenerator = !ts.isArrowFunction(node) && Boolean(node.asteriskToken); + if (isAsync || isGenerator) return 'value'; + return classifyFunction(node); } - if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node); + if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node, react); if (ts.isTaggedTemplateExpression(node)) return 'value'; if (ts.isCallExpression(node)) return isComponentWrapper(node.expression, react) ? 'component' : 'value'; if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { @@ -433,7 +456,9 @@ function exportKindsOf( for (const statement of sourceFile.statements) { if (ts.isTypeAliasDeclaration(statement) || ts.isInterfaceDeclaration(statement)) { - if (isExported(statement)) kinds.set(statement.name.text, 'erased'); + // `export default interface Foo {}` is looked up as `default`, the way the function and class + // branches already key theirs. + if (isExported(statement)) kinds.set(isDefault(statement) ? 'default' : statement.name.text, 'erased'); continue; } @@ -480,6 +505,14 @@ function exportKindsOf( continue; } + // `export * as Ns from './other'` is a namespace object. Recorded as a value, because every + // property read off one belonging to a client module is a client reference — and recorded at all, + // because a barrel over this barrel would otherwise lose the binding entirely. + if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamespaceExport(statement.exportClause)) { + if (!statement.isTypeOnly) kinds.set(statement.exportClause.name.text, 'value'); + continue; + } + if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamedExports(statement.exportClause)) { /* * `export { a }` is declared here. `export { a } from './b'` is declared in `./b`, and a @@ -874,7 +907,14 @@ describe('exportKindsOf', () => { ['an arrow', 'export const Subject = () =>
;'], ['memo imported from react', "import { memo } from 'react';\nexport const Subject = memo(() =>
);"], ['React.forwardRef', "import * as React from 'react';\nexport const Subject = React.forwardRef(() =>
);"], - ['a class extending Component', 'export class Subject extends React.Component {}'], + [ + 'a class extending React.Component', + "import * as React from 'react';\nexport class Subject extends React.Component {}", + ], + [ + 'a class extending an aliased React base', + "import { Component as Base } from 'react';\nexport class Subject extends Base {}", + ], ['an identifier default', 'const Inner = () =>
;\nexport default Inner;'], ['a parenthesised arrow', 'export const Subject = ((props) =>
);'], ['an arrow returning a component call', 'export const Subject = () => renderThing();'], @@ -905,6 +945,14 @@ describe('exportKindsOf', () => { "a wrapper call that is not React's", "import { memo } from 'other';\nexport const Subject = memo(() =>
);", ], + [ + 'a class extending an unrelated Component', + "import { Component } from './ui';\nexport class Subject extends Component {}", + ], + // A client component can be neither: one hands back a promise, the other an iterator. + ['an async function', 'export async function Subject() { return fetchThing(); }'], + ['an async arrow', 'export const Subject = async () =>
;'], + ['a generator function', 'export function* Subject() { yield item; }'], ['a default object', 'export default { a: 1 };'], ])('rejects %s', (_label, source) => { expect(kindOf(source, source.includes('export default') ? 'default' : 'Subject')).toBe('value'); @@ -946,6 +994,17 @@ describe('exportKindsOf', () => { expect(kindOf('export type Subject = { a: 1 };')).toBe('erased'); expect(kindOf('export interface Subject { a: 1 }')).toBe('erased'); expect(kindOf("export type { Subject } from './x';")).toBe('erased'); + // Keyed as `default`, which is what a default import looks it up as. + expect(kindOf('export default interface Subject { a: 1 }', 'default')).toBe('erased'); + }); + + it('records a namespace export, so a barrel over it keeps the binding', () => { + // Nothing to classify in `* as Ns` itself — every property read off one belonging to a client + // module is a client reference — but leaving it out of the map loses it for anything that + // re-exports this module. + const kinds = exportKindsOf(parse('barrel.tsx', "export * as Ns from './other';"), () => null); + + expect(kinds.get('Ns')).toBe('value'); }); it('follows a re-export to whatever the origin says it is', () => { From 74da6158618acc86800e359b8519b0f15755ffc7 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 17:27:17 -0700 Subject: [PATCH 11/23] test: decide once whether a binding may cross, and finish three half-pairs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings, all sound, all dormant — and four of them are the same shape: something I fixed on one side of a pair and not the other. The two paths that decide whether a binding is an offence had drifted apart. The star-re-export path exempted anything classified as a component before asking about capitalisation, so a lowercase export crossed through a barrel while the same name imported directly was rejected. The direct path asked about capitalisation before resolving the kind, so a lowercase erased type — `export interface options {}` — was reported as a runtime value. One `verdictFor` now, three questions in one order: erased crosses, lowercase never does, a component crosses. It is at module scope so the fixtures call it rather than restate it, which the first version of those fixtures did — they asserted a copy of the implementation and would have passed against anything. The other three pairs. `classify` unwrapped parentheses, `as` and `satisfies` while `returnsAValue` also unwrapped `!`; both do now. `import {}` was recorded as a side-effect edge last round and `export {} from` was not; both are. And `reactBindingsOf` took a default React import but not `import { default as React }`, which is the same import wearing a named spelling. Two more. `lazy` produces a component as surely as `memo` does, so it joins the binding-aware wrapper set — with a fixture for an unrelated `lazy` that does not. And a literal `require()` is an edge: there is one in this tree, in `markdown-adapter.ts`, and while nothing server-reachable imports that module today, the subtree past it would be unguarded the day something does. 55 tests. Five mutations on the classifier and the verdict, each failing only its own, and the two graph edges proved by planting a helper reachable through nothing else. --- ...client-values-in-server-boundaries.test.ts | 162 +++++++++++++----- 1 file changed, 117 insertions(+), 45 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 51d63ab273..2b3aa0a38b 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -88,7 +88,13 @@ function dynamicImportSpecifiers(sourceFile: ts.SourceFile): string[] { const found: string[] = []; const visit = (node: ts.Node) => { - if (ts.isCallExpression(node) && node.expression.kind === ts.SyntaxKind.ImportKeyword) { + // `require('…')` counts as well. There is one in this tree — `markdown-adapter.ts` reaches the + // tiptap extensions that way — and while nothing server-reachable imports that module today, + // the call is real and the subtree beyond it would be unguarded the day something does. + const isRequire = + ts.isCallExpression(node) && ts.isIdentifier(node.expression) && node.expression.text === 'require'; + + if (ts.isCallExpression(node) && (node.expression.kind === ts.SyntaxKind.ImportKeyword || isRequire)) { const [specifier] = node.arguments; // `isStringLiteralLike`, unlike the directive check: `import(`./helper`)` is a legal and // statically resolvable specifier, where `` `use client` `` is not a legal directive. A @@ -199,7 +205,7 @@ type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; */ /** `memo(X)` and `forwardRef(X)`, plain or `React.`-qualified, produce components. Nothing else does. */ -const COMPONENT_WRAPPERS = new Set(['memo', 'forwardRef']); +const COMPONENT_WRAPPERS = new Set(['memo', 'forwardRef', 'lazy']); /** The local names in one module that actually refer to React's wrappers and base classes. */ type ReactBindings = { wrappers: Set; bases: Set; namespaces: Set }; @@ -235,6 +241,9 @@ function reactBindingsOf(sourceFile: ts.SourceFile): ReactBindings { for (const element of clause.namedBindings.elements) { if (element.isTypeOnly) continue; const exported = (element.propertyName ?? element.name).text; + // `import { default as React } from 'react'` is a default import in named clothing, so the + // binding it makes is a namespace like any other default React import. + if (exported === 'default') namespaces.add(element.name.text); if (COMPONENT_WRAPPERS.has(exported)) wrappers.add(element.name.text); if (COMPONENT_BASES.has(exported)) bases.add(element.name.text); } @@ -429,7 +438,14 @@ function exportKindsOf( if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node, react); if (ts.isTaggedTemplateExpression(node)) return 'value'; if (ts.isCallExpression(node)) return isComponentWrapper(node.expression, react) ? 'component' : 'value'; - if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node) || ts.isSatisfiesExpression(node)) { + // The same transparent wrappers `returnsAValue` sees through, including `!`, which this half + // was missing — the two halves of one classifier disagreeing is how several of these started. + if ( + ts.isParenthesizedExpression(node) || + ts.isAsExpression(node) || + ts.isSatisfiesExpression(node) || + ts.isNonNullExpression(node) + ) { return classify(node.expression, seen); } if (ts.isIdentifier(node)) { @@ -706,6 +722,13 @@ describe('server components take only components from client modules', () => { reexported: true, }); } else { + // An empty list still evaluates the target, the same way `import {}` does. This is that + // check's other half, and it should have gone in with it. + if (statement.exportClause.elements.length === 0) { + found.push({ specifier, local: '' }); + continue; + } + for (const element of statement.exportClause.elements) { if (element.isTypeOnly) continue; found.push({ @@ -757,17 +780,12 @@ describe('server components take only components from client modules', () => { expect(serverGraph.size).toBeGreaterThan(100); }); - /** Only a capitalised name can be a component. `useFeatureFlag` is a function and still a value. */ - function isCapitalised(name: string): boolean { - return /^[A-Z]/.test(name) && name !== name.toUpperCase(); - } - /** * Every binding a server-graph module takes from a client module, with what the source says it * is. A name that is not capitalised cannot be a component whatever its declaration says, so it * is reported without asking. */ - function* clientBindings(): Generator<{ offence: string; kind: ExportKind; capitalised: boolean }> { + function* clientBindings(): Generator<{ offence: string | null; kind: ExportKind; capitalised: boolean }> { for (const file of serverGraph) { const from = file.split(path.sep).join('/'); @@ -789,55 +807,36 @@ describe('server components take only components from client modules', () => { for (const [exported, kind] of kindsFor(target)) { // `export *` does not carry the default export, so reading one here invents an offence. if (exported === 'default') continue; - if (kind === 'component' || kind === 'erased') continue; - if (isCapitalised(exported)) { - const label = kind === 'unknown' ? 'unclassifiable ' : ''; - yield { offence: `${from} -> re-exports ${exported} (${label}from ${source})`, kind, capitalised: true }; - } else { - yield { - offence: `${from} -> re-exports ${exported} (from ${source})`, - kind: 'value', - capitalised: false, - }; - } + + const verdict = verdictFor(exported, kind); + + yield { + offence: verdict ? `${from} -> re-exports ${exported} (${verdict.label}from ${source})` : null, + kind, + capitalised: verdict?.capitalised ?? isCapitalised(exported), + }; } continue; } if (!reference.exported) continue; - /* - * The gate reads the exported name, because that is the declaration being classified and - * React cannot render a lowercase binding as an element. A default import is judged by its - * local name instead, which is the only name it has. - * - * `export { default } from './client'` has neither — `default` is not a name anyone chose — - * so there is nothing for capitalisation to say and the declaration decides alone. Review - * also asked for `{ panel as Panel }` to be judged by the local alias; that one is declined - * below, in the reply, because the opposite alias `{ Button as button }` wants the opposite - * rule and neither spelling exists here. - */ const named = reference.exported === 'default' ? reference.local : reference.exported; - if (named !== 'default' && !isCapitalised(named)) { - yield { - offence: `${from} -> ${reference.local} (from ${source})${suffix}`, - kind: 'value', - capitalised: false, - }; - continue; - } - const kind = kindsFor(target).get(reference.exported) ?? 'unknown'; - const label = kind === 'unknown' ? `${reference.local} (unclassifiable` : `${reference.local} (`; - yield { offence: `${from} -> ${label}from ${source})${suffix}`, kind, capitalised: true }; + const verdict = verdictFor(named, kind); + + yield { + offence: verdict ? `${from} -> ${reference.local} (${verdict.label}from ${source})${suffix}` : null, + kind, + capitalised: verdict?.capitalised ?? isCapitalised(named), + }; } } } function* clientValuesInServerGraph(): Generator { - for (const { offence, kind } of clientBindings()) { - if (kind === 'component' || kind === 'erased') continue; - yield offence; + for (const { offence } of clientBindings()) { + if (offence) yield offence; } } @@ -896,6 +895,68 @@ describe('server components take only components from client modules', () => { * changelog. They were verified by hand at the time, by planting them in the tree and watching the * previous version disagree; written down here, they stay verified. */ +/** + * The verdict rule, which decides whether a binding may cross the boundary. + * + * Its own describe because it had two callers that disagreed: one exempted components before + * asking about capitalisation, the other asked about capitalisation before resolving the kind. The + * cases below are the disagreement, written down. + */ +/** Only a capitalised name can be a component. `useFeatureFlag` is a function and still a value. */ +function isCapitalised(name: string): boolean { + return /^[A-Z]/.test(name) && name !== name.toUpperCase(); +} + +/** + * Whether this binding is an offence, and how to describe it — or nothing, if it may cross. + * + * Written once because the two callers had drifted. The star-re-export path exempted anything + * classified as a component *before* asking about capitalisation, so a lowercase export slipped + * through a barrel while the same name imported directly was rejected; and the direct path asked + * about capitalisation *before* resolving the kind, so a lowercase erased type was reported as a + * runtime value. One order, three questions: erased crosses, lowercase never does, a component + * crosses. + * + * `default` is exempt from the capitalisation question because it is not a name anyone chose. + */ +function verdictFor(named: string, kind: ExportKind): { label: string; capitalised: boolean } | null { + if (kind === 'erased') return null; + if (named !== 'default' && !isCapitalised(named)) return { label: '', capitalised: false }; + if (kind === 'component') return null; + + return { label: kind === 'unknown' ? 'unclassifiable ' : '', capitalised: true }; +} + +describe('verdictFor', () => { + const offends = (named: string, kind: ExportKind) => verdictFor(named, kind) !== null; + + it('lets an erased export cross whatever it is called', () => { + // `export interface options {}` is gone before anything runs, so its lowercase name is not a + // reason to report it. This order was the other way round, and reported it. + expect(offends('options', 'erased')).toBe(false); + expect(offends('Options', 'erased')).toBe(false); + }); + + it('rejects a lowercase name even when the source calls it a component', () => { + // React cannot render ``, so a lowercase export is a value however it is + // declared — and it has to be rejected through a barrel as surely as it is directly. The star + // path exempted it, because it asked about the kind first. + expect(offends('buildOptions', 'component')).toBe(true); + expect(offends('BuildOptions', 'component')).toBe(false); + }); + + it('rejects a value and an unclassifiable export', () => { + expect(offends('Thing', 'value')).toBe(true); + expect(offends('Thing', 'unknown')).toBe(true); + expect(verdictFor('Thing', 'unknown')?.label).toBe('unclassifiable '); + }); + + it('asks nothing about the capitalisation of `default`', () => { + expect(offends('default', 'component')).toBe(false); + expect(offends('default', 'value')).toBe(true); + }); +}); + describe('exportKindsOf', () => { const kindOf = (source: string, exported = 'Subject') => exportKindsOf(parse('fixture.tsx', source), () => null).get(exported); @@ -915,6 +976,13 @@ describe('exportKindsOf', () => { 'a class extending an aliased React base', "import { Component as Base } from 'react';\nexport class Subject extends Base {}", ], + // `lazy` produces a component as surely as `memo` does. + ['lazy', "import { lazy } from 'react';\nexport const Subject = lazy(() => import('./panel'));"], + [ + 'a wrapper reached through a default-as-named React import', + "import { default as React } from 'react';\nexport const Subject = React.memo(() =>
);", + ], + ['a non-null asserted arrow', 'export const Subject = (() =>
)!;'], ['an identifier default', 'const Inner = () =>
;\nexport default Inner;'], ['a parenthesised arrow', 'export const Subject = ((props) =>
);'], ['an arrow returning a component call', 'export const Subject = () => renderThing();'], @@ -998,6 +1066,10 @@ describe('exportKindsOf', () => { expect(kindOf('export default interface Subject { a: 1 }', 'default')).toBe('erased'); }); + it("does not take an unrelated lazy for React's", () => { + expect(kindOf("import { lazy } from 'other';\nexport const Subject = lazy(() => 1);")).toBe('value'); + }); + it('records a namespace export, so a barrel over it keeps the binding', () => { // Nothing to classify in `* as Ns` itself — every property read off one belonging to a client // module is a client reference — but leaving it out of the map loses it for anything that From 426f4491a255b62e542f54a9d6a983b50ab65afa Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 17:49:01 -0700 Subject: [PATCH 12/23] test: stop the capitalisation gate standing in for classification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all sound, and two of them are the same mistake: a heuristic still deciding something the classifier now decides properly. All-uppercase names were excluded from "capitalised" on the grounds that `BOARD_GRID_CLASS` is obviously a constant. That also rejected `FAQ` and `A`, which are perfectly good component names, and it was standing in for classification that exists now — those three constants reach `verdictFor` and are reported for being values, which is both true and why they were on the allowlist to begin with. The corpus assertion excludes the allowlist rather than loosening its bound, so it keeps its teeth. A default import was judged by its local alias rather than by `default`, so a default client component imported under a lowercase name was rejected before its kind was considered — while the fixtures claimed `default` is exempt from the capitalisation question. The claim was right and the call site was wrong. And a returned function is as unrenderable as a returned object: `function BuildOptions() { return () => {}; }` is a factory, not a component. Proof, and one gap in it. Two mutations fail their own fixtures. The third — passing the local alias again — fails nothing, because that line needs a real module graph and the `verdictFor` fixtures only prove `default` is exempt once it gets there. Verified by planting a lowercase-aliased default import instead: reported before, silent after. That is written next to the line rather than left implied, because a fix with no failing test behind it should say so. --- ...client-values-in-server-boundaries.test.ts | 62 ++++++++++++++++--- 1 file changed, 53 insertions(+), 9 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 2b3aa0a38b..4082c3d2ed 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -336,7 +336,12 @@ function returnsAValue(expression: ts.Expression): boolean { return ( ts.isObjectLiteralExpression(expression) || ts.isRegularExpressionLiteral(expression) || - ts.isNewExpression(expression) + ts.isNewExpression(expression) || + // A function is not renderable either. `function BuildOptions() { return () => {}; }` is a + // factory, and so is anything handing back a class. + ts.isArrowFunction(expression) || + ts.isFunctionExpression(expression) || + ts.isClassExpression(expression) ); } @@ -821,14 +826,25 @@ describe('server components take only components from client modules', () => { if (!reference.exported) continue; - const named = reference.exported === 'default' ? reference.local : reference.exported; const kind = kindsFor(target).get(reference.exported) ?? 'unknown'; - const verdict = verdictFor(named, kind); + /* + * Judged by the exported name, which is the declaration being classified. For a default + * that is the word `default`, which `verdictFor` exempts from the capitalisation question + * because nobody chose it — the local alias is a fact about this file, not about the export. + * + * Passing the alias here instead rejected a default client component imported under a + * lowercase name, before its kind was ever considered. No fixture covers this line: it + * needs a real module graph, and the `verdictFor` tests below only prove that `default` is + * exempt once it gets there. Verified by planting `import suggestedFormats from + * '~/design-system/suggested-formats-window'` in the space layout — reported before, silent + * after. + */ + const verdict = verdictFor(reference.exported, kind); yield { offence: verdict ? `${from} -> ${reference.local} (${verdict.label}from ${source})${suffix}` : null, kind, - capitalised: verdict?.capitalised ?? isCapitalised(named), + capitalised: verdict?.capitalised ?? isCapitalised(reference.exported), }; } } @@ -870,14 +886,22 @@ describe('server components take only components from client modules', () => { */ it('classifies every capitalised client export the server graph takes', () => { const kinds: Record = { component: 0, erased: 0, value: 0, unknown: 0 }; - for (const { kind, capitalised } of clientBindings()) if (capitalised) kinds[kind] += 1; + + // The allowlist is excluded rather than the bound loosened. Three of its entries are + // all-uppercase and reach the classifier now that the capitalisation gate no longer pretends + // to do its job — they are values, correctly, which is why they are listed. + for (const { kind, capitalised, offence } of clientBindings()) { + if (!capitalised) continue; + if (offence && KNOWN.has(offence)) continue; + kinds[kind] += 1; + } // Components, and the two `export type`s imported without the `type` keyword — `Tabs` from // `editor-provider` and `Feature` from `use-place-search`. expect(kinds.component).toBeGreaterThan(100); expect(kinds.erased).toBe(2); - // Nothing capitalised is a value or unreadable. A classifier that started calling components - // values would move this off zero before the offence list above grew past its allowlist. + // Nothing else capitalised is a value or unreadable. A classifier that started calling + // components values would move this off zero before the offence list above grew. expect(kinds.value).toBe(0); expect(kinds.unknown).toBe(0); }); @@ -902,9 +926,18 @@ describe('server components take only components from client modules', () => { * asking about capitalisation, the other asked about capitalisation before resolving the kind. The * cases below are the disagreement, written down. */ -/** Only a capitalised name can be a component. `useFeatureFlag` is a function and still a value. */ +/** + * Only a capitalised name can be a component: React reads a lowercase tag as an HTML element, so + * `useFeatureFlag` is a function and still a value. + * + * All-uppercase names used to be excluded here as well, on the grounds that `BOARD_GRID_CLASS` is + * obviously a constant — which also rejected `FAQ` and `A`, both of which are perfectly good + * component names. The exclusion was standing in for classification, and there is real + * classification now: those three constants reach `verdictFor` and are reported for being values, + * which is both true and the reason they were on the allowlist to begin with. + */ function isCapitalised(name: string): boolean { - return /^[A-Z]/.test(name) && name !== name.toUpperCase(); + return /^[A-Z]/.test(name); } /** @@ -955,6 +988,14 @@ describe('verdictFor', () => { expect(offends('default', 'component')).toBe(false); expect(offends('default', 'value')).toBe(true); }); + + it('lets an acronym through and still reports a constant', () => { + // These two used to be answered by the same rule — an all-uppercase name was not capitalised, + // so `FAQ` was reported alongside `BOARD_GRID_CLASS`. The kind separates them now. + expect(offends('FAQ', 'component')).toBe(false); + expect(offends('A', 'component')).toBe(false); + expect(offends('BOARD_GRID_CLASS', 'value')).toBe(true); + }); }); describe('exportKindsOf', () => { @@ -1009,6 +1050,9 @@ describe('exportKindsOf', () => { ['a concise arrow returning an asserted object', 'export const Subject = () => ({}) as Thing;'], ['a function returning a constructed object', 'export function Subject() { return new Date(); }'], ['a function returning a regular expression', 'export function Subject() { return /x/; }'], + // A returned function is as unrenderable as a returned object. + ['a function returning a function', 'export function Subject() { return () => {}; }'], + ['a function returning a class', 'export function Subject() { return class {}; }'], [ "a wrapper call that is not React's", "import { memo } from 'other';\nexport const Subject = memo(() =>
);", From 475b30d42cf8069b70160bc097f4cd483af0e878 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 18:09:10 -0700 Subject: [PATCH 13/23] test: read every name a binding has, and every binding a require takes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all sound, all dormant — and one of them I had declined twice before on reasoning that turns out to be wrong. I twice refused to judge `{ widget as Widget }` by its alias, on the grounds that the opposite spelling `{ Button as button }` wants the opposite rule. That is true and it is not a reason to pick either: the gate can read *both* names. It does now, and both directions pass — `` renders, and a component under a lowercase alias is still a client component reference, which is the thing the boundary exists to allow. The cost is `{ useThing as Thing }`, a hook wearing a component's alias, which nothing here does and which no classifier can catch, since a hook is a function like any other. Reading one name was a false accusation; reading both is a dormant blind spot, and that is the better trade. That signature change also closes the hole from last commit. Judging a default by its local alias was a fix with no failing test behind it, because the call site needed a real module graph; passing the names as a list makes it a `verdictFor` case, and `['default', 'suggestedFormats']` is now a fixture. `require()` carries bindings. Recorded as a bare edge, `require('./client').VALUE` and `const { VALUE } = require('./client')` were traversed and then skipped, because the check needs a binding to look at. All three shapes are bindings now — a property read, a destructured name, and a whole module object for `const ns = require(…)` — with a bare call left as an edge, since it takes nothing. Proved by planting each of the three. A conditional or logical return is a value only if every branch is one: `enabled ? {} : {}` hands back an object either way, `cached ?? {}` does not, because `cached` could render. My first fixture for the second case asserted the wrong answer and the code was right. And `export namespace Foo {}` builds an object at runtime, so it is a value; `export declare namespace` is erased. 65 tests. Four mutations, each failing only its own fixtures, and the three require shapes planted end to end. --- ...client-values-in-server-boundaries.test.ts | 175 ++++++++++++++---- 1 file changed, 138 insertions(+), 37 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 4082c3d2ed..6d9ffdfb8a 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -75,31 +75,30 @@ const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image|robots|sitemap|manifest|icon|apple-icon|twitter-image)\.tsx?$/; /** - * The specifiers of `import('…')` calls, found in the tree rather than in the text. + * What a module pulls in through a call rather than a declaration: `import('…')` and `require('…')`. * - * Reading these off the source matched them inside comments, strings and template literals, and - * matched `type T = import('./types').T` — an `ImportTypeNode`, which TypeScript erases — as a - * runtime edge. It also missed any spelling with a comment in the middle. A `CallExpression` whose - * callee is the `import` keyword is none of those things by construction. + * Both are read from the tree rather than the text, so a spelling inside a comment or a string is + * not an edge and `type T = import('./types').T` — an `ImportTypeNode`, which TypeScript erases — + * is not one either. A computed specifier resolves to no file and is skipped. * - * Walking every node of all 1,633 files costs 86ms, which was the only argument for the regex. + * `require` also carries bindings, which a plain edge loses. The tree has one — `markdown-adapter` + * reaches the tiptap extensions as `require('…').tiptapExtensions` — and recorded as an edge alone + * that property read was invisible to the check even while the module was traversed. */ -function dynamicImportSpecifiers(sourceFile: ts.SourceFile): string[] { - const found: string[] = []; +function callReferences(sourceFile: ts.SourceFile): CallReference[] { + const found: CallReference[] = []; const visit = (node: ts.Node) => { - // `require('…')` counts as well. There is one in this tree — `markdown-adapter.ts` reaches the - // tiptap extensions that way — and while nothing server-reachable imports that module today, - // the call is real and the subtree beyond it would be unguarded the day something does. - const isRequire = - ts.isCallExpression(node) && ts.isIdentifier(node.expression) && node.expression.text === 'require'; - - if (ts.isCallExpression(node) && (node.expression.kind === ts.SyntaxKind.ImportKeyword || isRequire)) { + if (ts.isCallExpression(node)) { + const isDynamicImport = node.expression.kind === ts.SyntaxKind.ImportKeyword; + const isRequire = ts.isIdentifier(node.expression) && node.expression.text === 'require'; const [specifier] = node.arguments; - // `isStringLiteralLike`, unlike the directive check: `import(`./helper`)` is a legal and - // statically resolvable specifier, where `` `use client` `` is not a legal directive. A - // computed specifier resolves to no file, so there is nothing to follow. - if (specifier && ts.isStringLiteralLike(specifier)) found.push(specifier.text); + + if ((isDynamicImport || isRequire) && specifier && ts.isStringLiteralLike(specifier)) { + // `const { x } = await import('…')` is not destructured — see the file's doc block — so a + // dynamic import stays an edge and only `require` contributes bindings. + found.push(...(isRequire ? requireBindings(node, specifier.text) : [{ specifier: specifier.text, local: '' }])); + } } ts.forEachChild(node, visit); }; @@ -108,6 +107,42 @@ function dynamicImportSpecifiers(sourceFile: ts.SourceFile): string[] { return found; } +/** What a `require()` call's surroundings say is being taken from it. */ +type CallReference = { specifier: string; exported?: string; local: string; namespace?: boolean }; + +function requireBindings(call: ts.CallExpression, specifier: string): CallReference[] { + const parent = call.parent; + + // `require('./x').thing` + if (parent && ts.isPropertyAccessExpression(parent) && parent.expression === call) { + return [{ specifier, exported: parent.name.text, local: parent.name.text }]; + } + + if (parent && ts.isVariableDeclaration(parent) && parent.initializer === call) { + // `const { A, B } = require('./x')` + if (ts.isObjectBindingPattern(parent.name)) { + return parent.name.elements + .filter(element => ts.isIdentifier(element.name)) + .map(element => ({ + specifier, + exported: ts.isIdentifier(element.propertyName ?? element.name) + ? ((element.propertyName ?? element.name) as ts.Identifier).text + : '', + local: (element.name as ts.Identifier).text, + })) + .filter(reference => reference.exported); + } + + // `const ns = require('./x')` keeps the whole module object. + if (ts.isIdentifier(parent.name)) { + return [{ specifier, local: `* as ${parent.name.text}`, namespace: true }]; + } + } + + // Anything else — a bare call for its side effects — loads the module and takes nothing. + return [{ specifier, local: '' }]; +} + /** * What the tree holds today, each one read before being listed rather than swept up by the walk. * @@ -333,6 +368,22 @@ function returnsAValue(expression: ts.Expression): boolean { return returnsAValue(expression.expression); } + // Every branch, or it is not definite. `return enabled ? {} : {}` hands back an object either + // way; `cond ? {} :
` does not, and stays a component. + if (ts.isConditionalExpression(expression)) { + return returnsAValue(expression.whenTrue) && returnsAValue(expression.whenFalse); + } + + // `a ?? {}` and `a || {}` are the same question with two operands. + if ( + ts.isBinaryExpression(expression) && + [ts.SyntaxKind.QuestionQuestionToken, ts.SyntaxKind.BarBarToken, ts.SyntaxKind.AmpersandAmpersandToken].includes( + expression.operatorToken.kind + ) + ) { + return returnsAValue(expression.left) && returnsAValue(expression.right); + } + return ( ts.isObjectLiteralExpression(expression) || ts.isRegularExpressionLiteral(expression) || @@ -483,6 +534,13 @@ function exportKindsOf( continue; } + // `export namespace Foo {}` builds an object at runtime, so it is a value. An ambient one — + // `export declare namespace` — is erased like a type. + if (ts.isModuleDeclaration(statement) && isExported(statement) && ts.isIdentifier(statement.name)) { + kinds.set(statement.name.text, hasModifier(statement, ts.SyntaxKind.DeclareKeyword) ? 'erased' : 'value'); + continue; + } + // An enum is an object at runtime, whatever it looks like in the types. if (ts.isEnumDeclaration(statement) && isExported(statement)) { kinds.set(statement.name.text, 'value'); @@ -747,7 +805,7 @@ describe('server components take only components from client modules', () => { } } - for (const specifier of dynamicImportSpecifiers(sourceFile)) found.push({ specifier, local: '' }); + found.push(...callReferences(sourceFile)); referencesByFile.set(file, found); return found; @@ -813,7 +871,8 @@ describe('server components take only components from client modules', () => { // `export *` does not carry the default export, so reading one here invents an offence. if (exported === 'default') continue; - const verdict = verdictFor(exported, kind); + // One name here: a re-export has no local binding in this file to read. + const verdict = verdictFor([exported], kind); yield { offence: verdict ? `${from} -> re-exports ${exported} (${verdict.label}from ${source})` : null, @@ -839,7 +898,7 @@ describe('server components take only components from client modules', () => { * '~/design-system/suggested-formats-window'` in the space layout — reported before, silent * after. */ - const verdict = verdictFor(reference.exported, kind); + const verdict = verdictFor([reference.exported, reference.local], kind); yield { offence: verdict ? `${from} -> ${reference.local} (${verdict.label}from ${source})${suffix}` : null, @@ -952,49 +1011,78 @@ function isCapitalised(name: string): boolean { * * `default` is exempt from the capitalisation question because it is not a name anyone chose. */ -function verdictFor(named: string, kind: ExportKind): { label: string; capitalised: boolean } | null { +function verdictFor(names: string[], kind: ExportKind): { label: string; capitalised: boolean } | null { if (kind === 'erased') return null; - if (named !== 'default' && !isCapitalised(named)) return { label: '', capitalised: false }; + + /* + * Every name the binding has, because an alias can supply the one that matters and the two + * directions want opposite rules. `import { widget as Widget }` renders as ``, so the + * local name is what makes it a component; `import { Button as button }` is React's component + * under a name this file cannot render as a tag, but it is still a client component reference and + * passing one across the boundary is what the boundary is for. Reading either name accepts both. + * + * The cost is `{ useThing as Thing }` — a hook wearing a component's alias — which nothing here + * does, and which no classifier can catch, because a hook is a function like any other. + * + * `default` is not a name anyone chose, so its presence exempts the binding outright. + */ + if (!names.some(name => name === 'default' || isCapitalised(name))) { + return { label: '', capitalised: false }; + } if (kind === 'component') return null; return { label: kind === 'unknown' ? 'unclassifiable ' : '', capitalised: true }; } describe('verdictFor', () => { - const offends = (named: string, kind: ExportKind) => verdictFor(named, kind) !== null; + const offends = (names: string[], kind: ExportKind) => verdictFor(names, kind) !== null; it('lets an erased export cross whatever it is called', () => { // `export interface options {}` is gone before anything runs, so its lowercase name is not a // reason to report it. This order was the other way round, and reported it. - expect(offends('options', 'erased')).toBe(false); - expect(offends('Options', 'erased')).toBe(false); + expect(offends(['options'], 'erased')).toBe(false); + expect(offends(['Options'], 'erased')).toBe(false); }); it('rejects a lowercase name even when the source calls it a component', () => { // React cannot render ``, so a lowercase export is a value however it is // declared — and it has to be rejected through a barrel as surely as it is directly. The star // path exempted it, because it asked about the kind first. - expect(offends('buildOptions', 'component')).toBe(true); - expect(offends('BuildOptions', 'component')).toBe(false); + expect(offends(['buildOptions'], 'component')).toBe(true); + expect(offends(['BuildOptions'], 'component')).toBe(false); }); it('rejects a value and an unclassifiable export', () => { - expect(offends('Thing', 'value')).toBe(true); - expect(offends('Thing', 'unknown')).toBe(true); - expect(verdictFor('Thing', 'unknown')?.label).toBe('unclassifiable '); + expect(offends(['Thing'], 'value')).toBe(true); + expect(offends(['Thing'], 'unknown')).toBe(true); + expect(verdictFor(['Thing'], 'unknown')?.label).toBe('unclassifiable '); }); it('asks nothing about the capitalisation of `default`', () => { - expect(offends('default', 'component')).toBe(false); - expect(offends('default', 'value')).toBe(true); + expect(offends(['default'], 'component')).toBe(false); + expect(offends(['default'], 'value')).toBe(true); + // A default import under a lowercase alias is still a default: the local name is a fact about + // the importing file, not about the export. Judging by the alias rejected real components, and + // this is the assertion that was missing when that happened. + expect(offends(['default', 'suggestedFormats'], 'component')).toBe(false); }); it('lets an acronym through and still reports a constant', () => { // These two used to be answered by the same rule — an all-uppercase name was not capitalised, // so `FAQ` was reported alongside `BOARD_GRID_CLASS`. The kind separates them now. - expect(offends('FAQ', 'component')).toBe(false); - expect(offends('A', 'component')).toBe(false); - expect(offends('BOARD_GRID_CLASS', 'value')).toBe(true); + expect(offends(['FAQ'], 'component')).toBe(false); + expect(offends(['A'], 'component')).toBe(false); + expect(offends(['BOARD_GRID_CLASS'], 'value')).toBe(true); + }); + + it('reads either name when an alias supplies the capital', () => { + // `import { widget as Widget }` renders as ``, and `import { Button as button }` is + // still a component reference. Both directions pass; a hook under either spelling does not. + expect(offends(['widget', 'Widget'], 'component')).toBe(false); + expect(offends(['Button', 'button'], 'component')).toBe(false); + expect(offends(['useThing', 'useThing'], 'component')).toBe(true); + // And an alias cannot launder a value. + expect(offends(['widget', 'Widget'], 'value')).toBe(true); }); }); @@ -1024,6 +1112,10 @@ describe('exportKindsOf', () => { "import { default as React } from 'react';\nexport const Subject = React.memo(() =>
);", ], ['a non-null asserted arrow', 'export const Subject = (() =>
)!;'], + // One branch renders, so nothing is definite. + ['a function returning an element from one branch', 'export function Subject() { return on ?
: {}; }'], + // `cached` could be anything, including something renderable, so this is not definite either. + ['a function returning an object after an unknown left side', 'export function Subject() { return cached ?? {}; }'], ['an identifier default', 'const Inner = () =>
;\nexport default Inner;'], ['a parenthesised arrow', 'export const Subject = ((props) =>
);'], ['an arrow returning a component call', 'export const Subject = () => renderThing();'], @@ -1053,6 +1145,10 @@ describe('exportKindsOf', () => { // A returned function is as unrenderable as a returned object. ['a function returning a function', 'export function Subject() { return () => {}; }'], ['a function returning a class', 'export function Subject() { return class {}; }'], + // Every branch, or it is not definite. + ['a function returning an object from both branches', 'export function Subject() { return on ? {} : {}; }'], + ['a function returning an object from both sides of ??', 'export function Subject() { return {} ?? {}; }'], + ['an exported namespace', 'export namespace Subject { export const a = 1; }'], [ "a wrapper call that is not React's", "import { memo } from 'other';\nexport const Subject = memo(() =>
);", @@ -1102,6 +1198,11 @@ describe('exportKindsOf', () => { expect(kinds.has('default')).toBe(false); }); + it('erases an ambient namespace and keeps a real one', () => { + // `export namespace` builds an object at runtime; `declare` does not build anything. + expect(kindOf('export declare namespace Subject { const a: number; }')).toBe('erased'); + }); + it('treats a type as erased, however it is written', () => { expect(kindOf('export type Subject = { a: 1 };')).toBe('erased'); expect(kindOf('export interface Subject { a: 1 }')).toBe('erased'); From 98946d9d9a59daa58ed593dbc14b81b09f8cd6aa Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Mon, 21 Sep 2026 18:45:25 -0700 Subject: [PATCH 14/23] test: one list of transparent wrappers, one rule for `declare` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all sound, all dormant, and three of them are lists that were short by one — which is now the recurring shape rather than any single bug. The transparent-wrapper list was missing the angle-bracket assertion, in both halves this time. It has been short four separate times, so there is one `isTransparent` and both classifiers call it; a `.ts` fixture covers `x`, which is not valid in `.tsx` and is why nothing caught it. `declare` was handled for `export declare namespace` last round and nowhere else, so an ambient class became a value, an ambient const became unclassifiable, and an ambient function became a component. One check before the branches now: `declare` describes rather than builds, so nothing of it survives compilation. `require()` lost bindings three ways. Bracket access and string-key destructuring were skipped, and `const Widget = require('./x').widget` recorded `widget` as both names — which the both-names gate added *last commit* then rejected, a false accusation I introduced while fixing another one. The surrounding variable's name is kept, and all three shapes are planted end to end. An unresolved `export *` left an empty map, so a barrel over that barrel checked no bindings and passed in silence. It records `*` as unknown and is reported. And the entry seeds were any matching basename anywhere: a future `core/error.ts` would have been walked as a route nothing imports. Restricted to `app/`, which is the only place Next's conventions mean anything. 77 tests. That last one has no observable effect on this tree — nothing entry-shaped lives outside `app/` — so it is tested directly on the pattern rather than left unproven, and mutating the restriction away fails four cases. --- ...client-values-in-server-boundaries.test.ts | 181 ++++++++++++++---- 1 file changed, 144 insertions(+), 37 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 6d9ffdfb8a..21f0523583 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -72,7 +72,7 @@ const SOURCE_DIRS = ['app', 'atoms', 'core', 'design-system', 'partials']; * the walk entirely. */ const SERVER_ENTRY = - /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image|robots|sitemap|manifest|icon|apple-icon|twitter-image)\.tsx?$/; + /^app\/(?:.*\/)?(layout|page|template|default|loading|error|not-found|route|opengraph-image|robots|sitemap|manifest|icon|apple-icon|twitter-image)\.tsx?$/; /** * What a module pulls in through a call rather than a declaration: `import('…')` and `require('…')`. @@ -110,27 +110,51 @@ function callReferences(sourceFile: ts.SourceFile): CallReference[] { /** What a `require()` call's surroundings say is being taken from it. */ type CallReference = { specifier: string; exported?: string; local: string; namespace?: boolean }; +/** `require('x').thing` and `require('x')['thing']` name the same export two ways. */ +function accessedName(access: ts.PropertyAccessExpression | ts.ElementAccessExpression): string | null { + if (ts.isPropertyAccessExpression(access)) return access.name.text; + + const argument = access.argumentExpression; + // A computed key names nothing this can resolve; a literal one names an export. + return ts.isStringLiteralLike(argument) ? argument.text : null; +} + function requireBindings(call: ts.CallExpression, specifier: string): CallReference[] { const parent = call.parent; - // `require('./x').thing` - if (parent && ts.isPropertyAccessExpression(parent) && parent.expression === call) { - return [{ specifier, exported: parent.name.text, local: parent.name.text }]; + // `require('./x').thing`, `require('./x')['thing']`, and either assigned to a name: + // `const Widget = require('./x').widget` is a component under a capital, and losing that name is + // how the both-names gate came to reject it. + if ( + parent && + (ts.isPropertyAccessExpression(parent) || ts.isElementAccessExpression(parent)) && + parent.expression === call + ) { + const exported = accessedName(parent); + if (!exported) return [{ specifier, local: '' }]; + + const assignedTo = parent.parent; + const local = + assignedTo && ts.isVariableDeclaration(assignedTo) && ts.isIdentifier(assignedTo.name) + ? assignedTo.name.text + : exported; + + return [{ specifier, exported, local }]; } if (parent && ts.isVariableDeclaration(parent) && parent.initializer === call) { - // `const { A, B } = require('./x')` + // `const { A, B } = require('./x')`, including the `{ 'A': a }` spelling. if (ts.isObjectBindingPattern(parent.name)) { - return parent.name.elements - .filter(element => ts.isIdentifier(element.name)) - .map(element => ({ - specifier, - exported: ts.isIdentifier(element.propertyName ?? element.name) - ? ((element.propertyName ?? element.name) as ts.Identifier).text - : '', - local: (element.name as ts.Identifier).text, - })) - .filter(reference => reference.exported); + const bindings: CallReference[] = []; + + for (const element of parent.name.elements) { + const key = element.propertyName ?? element.name; + const exported = ts.isIdentifier(key) || ts.isStringLiteralLike(key) ? key.text : null; + const local = ts.isIdentifier(element.name) ? element.name.text : exported; + if (exported && local) bindings.push({ specifier, exported, local }); + } + + return bindings; } // `const ns = require('./x')` keeps the whole module object. @@ -343,6 +367,26 @@ function classifyFunction(node: ts.SignatureDeclaration): ExportKind { return returned.every(returnsAValue) ? 'value' : 'component'; } +/** + * Wrappers that say nothing about what is inside them. + * + * Kept in one place because both classifiers have to see through the same set, and the list has + * been short by one four separate times — `!` in one half but not the other, and the angle-bracket + * `x` assertion in neither. A `.ts` module can still write that form. + */ +function isTransparent( + expression: ts.Expression +): expression is + ts.ParenthesizedExpression | ts.AsExpression | ts.SatisfiesExpression | ts.NonNullExpression | ts.TypeAssertion { + return ( + ts.isParenthesizedExpression(expression) || + ts.isAsExpression(expression) || + ts.isSatisfiesExpression(expression) || + ts.isNonNullExpression(expression) || + ts.isTypeAssertionExpression(expression) + ); +} + /** * A return React could not render, through whatever is wrapped around it. * @@ -359,14 +403,7 @@ function classifyFunction(node: ts.SignatureDeclaration): ExportKind { * rather than a component — a different question, answered by the initialiser classifier. */ function returnsAValue(expression: ts.Expression): boolean { - if ( - ts.isParenthesizedExpression(expression) || - ts.isAsExpression(expression) || - ts.isSatisfiesExpression(expression) || - ts.isNonNullExpression(expression) - ) { - return returnsAValue(expression.expression); - } + if (isTransparent(expression)) return returnsAValue(expression.expression); // Every branch, or it is not definite. `return enabled ? {} : {}` hands back an object either // way; `cond ? {} :
` does not, and stays a component. @@ -494,16 +531,7 @@ function exportKindsOf( if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return classifyClass(node, react); if (ts.isTaggedTemplateExpression(node)) return 'value'; if (ts.isCallExpression(node)) return isComponentWrapper(node.expression, react) ? 'component' : 'value'; - // The same transparent wrappers `returnsAValue` sees through, including `!`, which this half - // was missing — the two halves of one classifier disagreeing is how several of these started. - if ( - ts.isParenthesizedExpression(node) || - ts.isAsExpression(node) || - ts.isSatisfiesExpression(node) || - ts.isNonNullExpression(node) - ) { - return classify(node.expression, seen); - } + if (ts.isExpression(node) && isTransparent(node)) return classify(node.expression, seen); if (ts.isIdentifier(node)) { const local = locals.get(node.text); if (local) return classify(local, seen); @@ -534,10 +562,26 @@ function exportKindsOf( continue; } - // `export namespace Foo {}` builds an object at runtime, so it is a value. An ambient one — - // `export declare namespace` — is erased like a type. + /* + * `declare` means the declaration describes something that already exists rather than building + * it, so nothing of it survives compilation — a class, a const, a function and a namespace + * alike. This was written for `export declare namespace` alone last round and left the other + * three classified as runtime exports, which reported a type-only import as a client value. + */ + if (isExported(statement) && hasModifier(statement, ts.SyntaxKind.DeclareKeyword)) { + const declared: (ts.Node | undefined)[] = ts.isVariableStatement(statement) + ? statement.declarationList.declarations.map(declaration => declaration.name) + : [(statement as ts.DeclarationStatement).name]; + + for (const name of declared) { + if (name && ts.isIdentifier(name)) kinds.set(name.text, 'erased'); + } + continue; + } + + // `export namespace Foo {}` builds an object at runtime, so it is a value. if (ts.isModuleDeclaration(statement) && isExported(statement) && ts.isIdentifier(statement.name)) { - kinds.set(statement.name.text, hasModifier(statement, ts.SyntaxKind.DeclareKeyword) ? 'erased' : 'value'); + kinds.set(statement.name.text, 'value'); continue; } @@ -576,7 +620,17 @@ function exportKindsOf( if (ts.isExportDeclaration(statement) && !statement.exportClause && statement.moduleSpecifier) { if (statement.isTypeOnly || !ts.isStringLiteral(statement.moduleSpecifier)) continue; const origin = resolveOrigin(statement.moduleSpecifier.text); - if (!origin) continue; + + /* + * An unresolved target is not an empty one. Dropping it left the map with no bindings at all, + * so a server barrel star-re-exporting this one checked nothing and passed in silence — the + * worst shape of answer this guard can give. `*` is recorded instead, which reads as + * `re-exports *` and is reported. + */ + if (!origin) { + kinds.set('*', 'unknown'); + continue; + } for (const [name, kind] of origin) { if (name !== 'default') kinds.set(name, kind); @@ -1034,6 +1088,34 @@ function verdictFor(names: string[], kind: ExportKind): { label: string; capital return { label: kind === 'unknown' ? 'unclassifiable ' : '', capitalised: true }; } +/** + * Which files seed the walk. + * + * Tested directly on the pattern, because the tree holds no entry-shaped basename outside `app/` + * today — so restricting it changes nothing observable, and a fix with nothing to fail is a fix + * nobody can trust. A future `core/error.ts` or `partials/loading.tsx` would otherwise be walked as + * a route nothing imports, and offences found through it would be against code the server never + * renders. + */ +describe('SERVER_ENTRY', () => { + it.each([ + 'app/layout.tsx', + 'app/space/[id]/(space)/layout.tsx', + 'app/bounties/loading.tsx', + 'app/robots.ts', + 'app/api/chat/route.ts', + ])('seeds %s', file => { + expect(SERVER_ENTRY.test(file)).toBe(true); + }); + + it.each(['core/error.ts', 'partials/loading.tsx', 'design-system/page.tsx', 'atoms/route.ts'])( + 'does not seed %s', + file => { + expect(SERVER_ENTRY.test(file)).toBe(false); + } + ); +}); + describe('verdictFor', () => { const offends = (names: string[], kind: ExportKind) => verdictFor(names, kind) !== null; @@ -1198,6 +1280,31 @@ describe('exportKindsOf', () => { expect(kinds.has('default')).toBe(false); }); + it('sees through an angle-bracket type assertion', () => { + // `.ts`, because `x` is not valid in `.tsx` — which is why nothing caught this until now. + const kindOfTs = (source: string) => exportKindsOf(parse('fixture.ts', source), () => null).get('Subject'); + + expect(kindOfTs('export function Subject() { return >{}; }')).toBe('value'); + expect(kindOfTs('export const Subject = <() => null>(() => null);')).toBe('component'); + }); + + it('erases every ambient declaration, not just a namespace', () => { + // `declare` describes something that exists rather than building it, so none of these survive + // compilation. Only the namespace form was handled, and the other three were called runtime. + expect(kindOf('export declare class Subject {}')).toBe('erased'); + expect(kindOf('export declare const Subject: object;')).toBe('erased'); + expect(kindOf('export declare function Subject(): void;')).toBe('erased'); + expect(kindOf('export declare enum Subject { A }')).toBe('erased'); + }); + + it('keeps an unresolved wildcard rather than reporting nothing', () => { + // `export * from './missing'` used to leave an empty map, so a barrel over this barrel checked + // no bindings and passed in silence. `*` is recorded and reported instead. + const kinds = exportKindsOf(parse('barrel.tsx', "export * from './missing';"), () => null); + + expect(kinds.get('*')).toBe('unknown'); + }); + it('erases an ambient namespace and keeps a real one', () => { // `export namespace` builds an object at runtime; `declare` does not build anything. expect(kindOf('export declare namespace Subject { const a: number; }')).toBe('erased'); From 3617bc6b81d9397ec344665b3edb3dc882193c7f Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Thu, 24 Sep 2026 15:05:07 -0700 Subject: [PATCH 15/23] docs(test): say that `lazy` is a component wrapper too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was added to the set two rounds ago and the comment above it still listed `memo` and `forwardRef` — the small version of the mistake this file keeps making, a list extended in one place and described in another. No behaviour change, so nothing to prove: the set was already right. --- .../client-values-in-server-boundaries.test.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 21f0523583..061f34f628 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -22,7 +22,7 @@ import { describe, expect, it } from 'vitest'; * * ## Why this parses instead of matching * - * It used to match patterns near declarations rather than read them, and review found ten ways that + * It used to match patterns near declarations rather than read them, and review found a dozen ways that * was wrong. An unbounded matcher began on a bare CSS import and captured a later statement's * specifier. `import Default, * as Namespace` matched neither of two patterns. `'use client'` was * recognised only as the very first token, so a licence header would hide an entire client module — @@ -263,7 +263,14 @@ type ExportKind = 'component' | 'erased' | 'value' | 'unknown'; * tag, which is a visible failure rather than a silent one. */ -/** `memo(X)` and `forwardRef(X)`, plain or `React.`-qualified, produce components. Nothing else does. */ +/** + * The React calls that produce a component: `memo`, `forwardRef` and `lazy`, plain or + * `React.`-qualified. Every other call is a value. + * + * `lazy` arrived later than the other two and this comment did not, which is the small version of + * the mistake the rest of the file keeps making — a list extended in one place and described in + * another. + */ const COMPONENT_WRAPPERS = new Set(['memo', 'forwardRef', 'lazy']); /** The local names in one module that actually refer to React's wrappers and base classes. */ From 6750a66e382e9af5374d95333a17bb62116714f5 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Thu, 24 Sep 2026 16:25:25 -0700 Subject: [PATCH 16/23] test: catch a require-based re-export, and commit the probes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `export default require('./client')` fell through to the side-effect fallback and produced a reference with no binding, which `clientBindings` then skips — a barrel could hand a whole client module on and the guard would report nothing. An `ExportAssignment` now records the module object, the way `const ns = require(…)` already did. The other two are the same problem as the fix I shipped for the entry-point anchor: logic whose only evidence was a throwaway probe I ran once. Deleting the dynamic-import branch, or reading a backtick `use client` as a directive, left every corpus assertion green, because nothing in this tree exercises either path. `callReferences` and `isClientModule` now have fixtures, so the next person to simplify them finds out from the suite instead of from production. Each fixture group was checked by breaking the code it covers: nine mutations, nine failures, none of them in the corpus assertions. --- ...client-values-in-server-boundaries.test.ts | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 061f34f628..cd8147464c 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -163,6 +163,16 @@ function requireBindings(call: ts.CallExpression, specifier: string): CallRefere } } + /* + * `export default require('./x')` hands the whole module object on, and `export =` does the same + * wherever a bundler allows it. Both reached the fallback below and produced a reference with no + * binding, which `clientBindings` skips — so a barrel written this way re-exported a client module + * in silence. `export { x } from './x'` cannot arrive here: a call is never its direct child. + */ + if (parent && ts.isExportAssignment(parent) && parent.expression === call) { + return [{ specifier, local: parent.isExportEquals ? 'export =' : 'export default', namespace: true }]; + } + // Anything else — a bare call for its side effects — loads the module and takes nothing. return [{ specifier, local: '' }]; } @@ -1123,6 +1133,94 @@ describe('SERVER_ENTRY', () => { ); }); +describe('isClientModule', () => { + const isClient = (source: string) => isClientModule(parse('fixture.tsx', source)); + + it.each([ + ['on its own', "'use client';"], + ['double-quoted', '"use client";'], + ['behind a licence header and a blank line', "/* Copyright */\n// notes\n\n'use client';"], + ['behind another directive', "'use strict';\n'use client';"], + ])('reads the directive %s', (_label, source) => { + expect(isClient(source)).toBe(true); + }); + + it.each([ + ['a module with no directive', 'export const A = 1;'], + // A template literal is not a directive. Reading one as a directive moves a *server* module into + // the client set, which stops the walk at it and hides everything behind it. + ['a template literal spelling of it', '`use client`;'], + // The prologue ends at the first statement that is not a directive. + ['a directive after an import', "import './x';\n'use client';"], + ['the string used as an argument', "register('use client');"], + ])('does not read %s as opting into the client', (_label, source) => { + expect(isClient(source)).toBe(false); + }); +}); + +describe('callReferences', () => { + const refs = (source: string) => callReferences(parse('fixture.ts', source)); + + it.each([ + ['a string specifier', "void import('./panel');"], + ['a no-substitution template', 'void import(`./panel`);'], + ])('follows a dynamic import written with %s', (_label, source) => { + expect(refs(source)).toEqual([{ specifier: './panel', local: '' }]); + }); + + it.each([ + ['an interpolated specifier', 'void import(`./${name}`);'], + ['a variable specifier', 'void import(name);'], + // Read from the tree rather than the text: an `ImportTypeNode` is not a call, and TypeScript + // erases it, so following it would walk to a module that does not exist at runtime. + ['an import type', "type T = import('./panel').T;"], + ['a spelling inside a string', 'const source = "import(\'./panel\')";'], + ])('does not follow %s', (_label, source) => { + expect(refs(source)).toEqual([]); + }); + + it('reads both the export a require names and the name it is bound to', () => { + // Losing the local name is how the both-names gate came to reject a component under a capital. + expect(refs("const Widget = require('./x').widget;")).toEqual([ + { specifier: './x', exported: 'widget', local: 'Widget' }, + ]); + expect(refs("const Widget = require('./x')['widget'];")).toEqual([ + { specifier: './x', exported: 'widget', local: 'Widget' }, + ]); + }); + + it('falls back to the exported name when nothing is bound to it', () => { + expect(refs("use(require('./x').widget);")).toEqual([{ specifier: './x', exported: 'widget', local: 'widget' }]); + }); + + it('reads every binding a destructured require takes', () => { + expect(refs("const { A, B: b } = require('./x');")).toEqual([ + { specifier: './x', exported: 'A', local: 'A' }, + { specifier: './x', exported: 'B', local: 'b' }, + ]); + }); + + it('keeps the whole module object when a require is bound to one name', () => { + expect(refs("const ns = require('./x');")).toEqual([{ specifier: './x', local: '* as ns', namespace: true }]); + }); + + it.each([ + ['a call for its side effects', "require('./x');"], + ['a computed property read', "const thing = require('./x')[key];"], + ])('takes no binding from %s, but still walks the module', (_label, source) => { + expect(refs(source)).toEqual([{ specifier: './x', local: '' }]); + }); + + it.each([ + ['export default', "export default require('./x');", 'export default'], + ['export =', "export = require('./x');", 'export ='], + ])('records a %s require as the whole module object', (_label, source, local) => { + // Without this these produced a reference with no binding, which `clientBindings` skips — a + // barrel re-exporting a client module and nothing reported. + expect(refs(source)).toEqual([{ specifier: './x', local, namespace: true }]); + }); +}); + describe('verdictFor', () => { const offends = (names: string[], kind: ExportKind) => verdictFor(names, kind) !== null; From f7183695a687347c4b60a3de33b2d8d36566df47 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Thu, 24 Sep 2026 16:56:06 -0700 Subject: [PATCH 17/23] test: stop a merged type erasing the value it shares a name with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Types and values are separate declaration spaces, so one name can legally hold both, and `export const SortOrder = {…}` followed by `export type SortOrder = …` is the Effect idiom this tree uses seventeen times — in both orders, because nothing makes you pick one. The erased half was recorded with an unconditional `set`, so whichever came last won, and `erased` is the single kind `verdictFor` waves through. A client value written that way crossed the boundary with nothing reported: not a wrong answer, which is the failure this guard exists to prevent. Recording an erased declaration now leaves a runtime one of the same name alone. Only in that direction — a name with a runtime declaration has a runtime export, so a type never makes it erased, while the reverse order was already right. Both branches that can collide take the guard: types and interfaces, and `declare`, where `export function X` merging with `export declare namespace X` is legal. The third erased write keeps its plain `set`, because an explicit type-only clause has to be able to outrank an `export *` of the same name. `staticReferences` moves out of the suite to module scope for the same reason `callReferences` got fixtures last commit: inside the closure it was only ever exercised against this tree, where every bare import resolves to CSS or a package, so deleting that edge left every corpus assertion green. Its own doc block claimed the opposite of what it did. And `classifyFunction`'s comment listed arrays, strings and numbers as values, which is the behaviour an earlier round corrected — React renders all three. It now defers to `returnsAValue` rather than keeping a second copy of the list, which is the mistake this file has made four times. Thirteen mutations, thirteen failures. --- ...client-values-in-server-boundaries.test.ts | 359 ++++++++++++------ 1 file changed, 245 insertions(+), 114 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index cd8147464c..463da39268 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -177,6 +177,122 @@ function requireBindings(call: ts.CallExpression, specifier: string): CallRefere return [{ specifier, local: '' }]; } +/** One binding taken from another module: a named export, a default, or a whole namespace. */ +type Reference = { + specifier: string; + exported?: string; + local: string; + /** `import * as X` / `export * as X`: an object whose every property is a client reference. */ + namespace?: boolean; + /** `export * from`: the target's named bindings, each classified on its own. */ + starReexport?: boolean; + reexported?: boolean; +}; + +/** + * Every module a file pulls in at runtime, with the bindings it takes from each. + * + * Type-only imports and exports are skipped, whole-statement and per-element alike. That exclusion + * is load-bearing rather than tidy: the only route into `core/blocks/data/filters.ts` is a type + * import from `core/chat/edit-types.ts`, and following it walks on into the sync store and reports + * three modules TypeScript erases before anything runs. + * + * At module scope so the fixtures below can reach it. Inside the suite it was only ever exercised + * against this tree, where every bare import resolves to CSS or a package — so deleting that edge + * left the corpus green and reopened the gap it was added to close, which is the same hole the + * `callReferences` fixtures were written for. + */ +function staticReferences(sourceFile: ts.SourceFile): Reference[] { + const found: Reference[] = []; + + for (const statement of sourceFile.statements) { + if (ts.isImportDeclaration(statement) && ts.isStringLiteral(statement.moduleSpecifier)) { + const specifier = statement.moduleSpecifier.text; + const clause = statement.importClause; + + // A bare `import './x'` takes no bindings but still loads the module. + if (!clause) { + found.push({ specifier, local: '' }); + continue; + } + if (clause.isTypeOnly) continue; + + // `import {} from './x'` has a clause with nothing in it and still loads the module. Review + // also asked for the all-type-specifier form; that is declined in the reply, because this + // repo does not set `verbatimModuleSyntax` and TypeScript elides it. + if ( + !clause.name && + clause.namedBindings && + ts.isNamedImports(clause.namedBindings) && + clause.namedBindings.elements.length === 0 + ) { + found.push({ specifier, local: '' }); + continue; + } + + if (clause.name) found.push({ specifier, exported: 'default', local: clause.name.text }); + + if (clause.namedBindings && ts.isNamespaceImport(clause.namedBindings)) { + found.push({ specifier, local: `* as ${clause.namedBindings.name.text}`, namespace: true }); + } else if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { + for (const element of clause.namedBindings.elements) { + if (element.isTypeOnly) continue; + found.push({ + specifier, + exported: (element.propertyName ?? element.name).text, + local: element.name.text, + }); + } + } + continue; + } + + if ( + ts.isExportDeclaration(statement) && + statement.moduleSpecifier && + ts.isStringLiteral(statement.moduleSpecifier) + ) { + if (statement.isTypeOnly) continue; + const specifier = statement.moduleSpecifier.text; + + if (!statement.exportClause) { + // Not a namespace value: this hands on the target's named bindings one by one, so a + // barrel star-re-exporting nothing but components is not an offence. `export * as Ns` is + // a namespace object and stays one, below. + found.push({ specifier, local: 're-exports *', starReexport: true, reexported: true }); + } else if (ts.isNamespaceExport(statement.exportClause)) { + found.push({ + specifier, + local: `re-exports * as ${statement.exportClause.name.text}`, + namespace: true, + reexported: true, + }); + } else { + // An empty list still evaluates the target, the same way `import {}` does. This is that + // check's other half, and it should have gone in with it. + if (statement.exportClause.elements.length === 0) { + found.push({ specifier, local: '' }); + continue; + } + + for (const element of statement.exportClause.elements) { + if (element.isTypeOnly) continue; + found.push({ + specifier, + exported: (element.propertyName ?? element.name).text, + local: element.name.text, + reexported: true, + }); + } + } + } + } + + found.push(...callReferences(sourceFile)); + + return found; +} + /** * What the tree holds today, each one read before being listed rather than swept up by the walk. * @@ -349,11 +465,17 @@ function isComponentWrapper(expression: ts.Expression, react: ReactBindings): bo * `PersonalProfileBioStarterMerge` of being values. A guard that accuses real components is one * somebody deletes. * - * So the evidence runs the other way: a function whose every `return` hands back an object, array, - * string or number is a value — which is `function BuildOptions() { return {}; }`, the case review - * named — and anything else is left as a component. A concise arrow body counts as a return, or - * `() => ({})` slips through the same door the block form was just closed on, and a literal is - * recognised through parentheses, `as`, `satisfies` and `!`. + * So the evidence runs the other way: a function whose every `return` hands back something React + * cannot render is a value — `function BuildOptions() { return {}; }`, the case review named — and + * anything else is left as a component. What counts as unrenderable is `returnsAValue`'s to say and + * is not restated here; an earlier version of this comment listed arrays, strings and numbers among + * them, which is the opposite of what the classifier does and of what its fixtures assert. React + * renders all three, so a function returning one is a component. Two lists that had to be kept + * level is the mistake this file has already made four times. + * + * A concise arrow body counts as a return, or `() => ({})` slips through the same door the block + * form was just closed on, and a literal is recognised through parentheses, `as`, `satisfies` + * and `!`. * * Deciding from the returns alone also drops the separate JSX scan this used to run, which could * override a definite literal return — a function handing back `{ label: }` returns an @@ -496,6 +618,23 @@ function exportKindsOf( resolveOrigin: (specifier: string) => Map | null ): Map { const kinds = new Map(); + /** + * Records something TypeScript erases, without losing a runtime export of the same name. + * + * Types and values are separate declaration spaces, so one name can legally have both — which is + * the Effect idiom this tree uses 17 times: `export const SortOrder = {…}` followed by + * `export type SortOrder = …`. An unconditional `set` made the answer depend on which came last, + * and the schema modules here are written both ways round. The erased half overwriting the value + * is the direction that matters: `erased` is the one kind `verdictFor` waves through, so a client + * value written this way crossed the boundary with nothing reported. + * + * Only ever safe in this direction. A name with a runtime declaration has a runtime export, so a + * type of the same name never makes it erased — while the reverse order is already right, because + * the runtime kind is the true one whenever both exist. + */ + const recordErased = (name: string) => { + if (!kinds.has(name)) kinds.set(name, 'erased'); + }; const react = reactBindingsOf(sourceFile); /** Local declarations, so an export by identifier has something to resolve against. */ const locals = new Map(); @@ -575,7 +714,7 @@ function exportKindsOf( if (ts.isTypeAliasDeclaration(statement) || ts.isInterfaceDeclaration(statement)) { // `export default interface Foo {}` is looked up as `default`, the way the function and class // branches already key theirs. - if (isExported(statement)) kinds.set(isDefault(statement) ? 'default' : statement.name.text, 'erased'); + if (isExported(statement)) recordErased(isDefault(statement) ? 'default' : statement.name.text); continue; } @@ -591,7 +730,9 @@ function exportKindsOf( : [(statement as ts.DeclarationStatement).name]; for (const name of declared) { - if (name && ts.isIdentifier(name)) kinds.set(name.text, 'erased'); + // `export function X() {}` and `export declare namespace X {…}` merge, and that order is + // legal — checked with the compiler, not assumed. + if (name && ts.isIdentifier(name)) recordErased(name.text); } continue; } @@ -681,6 +822,11 @@ function exportKindsOf( for (const element of statement.exportClause.elements) { if (statement.isTypeOnly || element.isTypeOnly) { + // A plain `set`, not `recordErased`: an explicit clause outranks an `export *`, and + // `export * from './x'` beside `export type { Foo } from './x'` is legal, so this has to + // be able to overwrite what the star recorded. It cannot collide with a declaration in + // this module the way the two branches above can — `export const Foo` next to + // `export type { Foo }` is a duplicate export and does not compile. kinds.set(element.name.text, 'erased'); continue; } @@ -764,119 +910,14 @@ describe('server components take only components from client modules', () => { return null; } - /** One binding taken from another module: a named export, a default, or a whole namespace. */ - type Reference = { - specifier: string; - exported?: string; - local: string; - /** `import * as X` / `export * as X`: an object whose every property is a client reference. */ - namespace?: boolean; - /** `export * from`: the target's named bindings, each classified on its own. */ - starReexport?: boolean; - reexported?: boolean; - }; - const referencesByFile = new Map(); - /** - * Every module a file pulls in at runtime, with the bindings it takes from each. - * - * Type-only imports and exports are skipped, whole-statement and per-element alike. That - * exclusion is load-bearing rather than tidy: the only route into `core/blocks/data/filters.ts` - * is a type import from `core/chat/edit-types.ts`, and following it walks on into the sync store - * and reports three modules TypeScript erases before anything runs. - */ + /** `staticReferences`, memoised per file. */ function references(file: string): Reference[] { const cached = referencesByFile.get(file); if (cached) return cached; - const sourceFile = astByFile.get(file)!; - const found: Reference[] = []; - - for (const statement of sourceFile.statements) { - if (ts.isImportDeclaration(statement) && ts.isStringLiteral(statement.moduleSpecifier)) { - const specifier = statement.moduleSpecifier.text; - const clause = statement.importClause; - - // A bare `import './x'` takes no bindings but still loads the module. - if (!clause) { - found.push({ specifier, local: '' }); - continue; - } - if (clause.isTypeOnly) continue; - - // `import {} from './x'` has a clause with nothing in it and still loads the module. Review - // also asked for the all-type-specifier form; that is declined in the reply, because this - // repo does not set `verbatimModuleSyntax` and TypeScript elides it. - if ( - !clause.name && - clause.namedBindings && - ts.isNamedImports(clause.namedBindings) && - clause.namedBindings.elements.length === 0 - ) { - found.push({ specifier, local: '' }); - continue; - } - - if (clause.name) found.push({ specifier, exported: 'default', local: clause.name.text }); - - if (clause.namedBindings && ts.isNamespaceImport(clause.namedBindings)) { - found.push({ specifier, local: `* as ${clause.namedBindings.name.text}`, namespace: true }); - } else if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { - for (const element of clause.namedBindings.elements) { - if (element.isTypeOnly) continue; - found.push({ - specifier, - exported: (element.propertyName ?? element.name).text, - local: element.name.text, - }); - } - } - continue; - } - - if ( - ts.isExportDeclaration(statement) && - statement.moduleSpecifier && - ts.isStringLiteral(statement.moduleSpecifier) - ) { - if (statement.isTypeOnly) continue; - const specifier = statement.moduleSpecifier.text; - - if (!statement.exportClause) { - // Not a namespace value: this hands on the target's named bindings one by one, so a - // barrel star-re-exporting nothing but components is not an offence. `export * as Ns` is - // a namespace object and stays one, below. - found.push({ specifier, local: 're-exports *', starReexport: true, reexported: true }); - } else if (ts.isNamespaceExport(statement.exportClause)) { - found.push({ - specifier, - local: `re-exports * as ${statement.exportClause.name.text}`, - namespace: true, - reexported: true, - }); - } else { - // An empty list still evaluates the target, the same way `import {}` does. This is that - // check's other half, and it should have gone in with it. - if (statement.exportClause.elements.length === 0) { - found.push({ specifier, local: '' }); - continue; - } - - for (const element of statement.exportClause.elements) { - if (element.isTypeOnly) continue; - found.push({ - specifier, - exported: (element.propertyName ?? element.name).text, - local: element.name.text, - reexported: true, - }); - } - } - } - } - - found.push(...callReferences(sourceFile)); + const found = staticReferences(astByFile.get(file)!); referencesByFile.set(file, found); return found; @@ -1133,6 +1174,63 @@ describe('SERVER_ENTRY', () => { ); }); +describe('staticReferences', () => { + const refs = (source: string) => staticReferences(parse('fixture.tsx', source)); + + it.each([ + ['a bare import', "import './x';"], + ['an import with an empty list', "import {} from './x';"], + ['a re-export with an empty list', "export {} from './x';"], + ])('records %s as an edge that takes no binding', (_label, source) => { + // Dormant in this tree — every bare import here resolves to CSS or a package — so deleting any + // of these three leaves the corpus assertions green. That is what these fixtures are for. + expect(refs(source)).toEqual([{ specifier: './x', local: '' }]); + }); + + it.each([ + ['a type-only import', "import type { A } from './x';"], + ['a type-only specifier', "import { type A } from './x';"], + ['a type-only re-export', "export type { A } from './x';"], + ['a type-only re-export specifier', "export { type A } from './x';"], + ])('does not follow %s', (_label, source) => { + // Load-bearing: following type edges walks to three modules TypeScript erases before anything + // runs, and reports offences against code the server never executes. + expect(refs(source)).toEqual([]); + }); + + it('reads a default and a namespace from one statement', () => { + // `import Default, * as Namespace` matched neither of the two patterns an earlier version had. + expect(refs("import Panel, * as Everything from './x';")).toEqual([ + { specifier: './x', exported: 'default', local: 'Panel' }, + { specifier: './x', local: '* as Everything', namespace: true }, + ]); + }); + + it('keeps both names an aliased re-export has', () => { + expect(refs("export { Inner as Outer } from './x';")).toEqual([ + { specifier: './x', exported: 'Inner', local: 'Outer', reexported: true }, + ]); + }); + + it('tells a star re-export from a namespace one', () => { + // `export *` hands on named bindings one by one, so a barrel of components is not an offence. + // `export * as Ns` is a namespace object, and every property read off one is a client reference. + expect(refs("export * from './x';")).toEqual([ + { specifier: './x', local: 're-exports *', starReexport: true, reexported: true }, + ]); + expect(refs("export * as Ns from './x';")).toEqual([ + { specifier: './x', local: 're-exports * as Ns', namespace: true, reexported: true }, + ]); + }); + + it('reads call edges alongside the declared ones', () => { + expect(refs("import './a';\nvoid import('./b');")).toEqual([ + { specifier: './a', local: '' }, + { specifier: './b', local: '' }, + ]); + }); +}); + describe('isClientModule', () => { const isClient = (source: string) => isClientModule(parse('fixture.tsx', source)); @@ -1453,6 +1551,39 @@ describe('exportKindsOf', () => { expect(kindOf("export { Subject } from './somewhere-unreadable';")).toBe('unknown'); }); + it('does not let a merged type erase the value it shares a name with', () => { + // `export const X = {…}; export type X = …` is legal — separate declaration spaces — and is the + // Effect idiom this tree uses 17 times, in both orders. The erased half was written last and + // overwrote the value, and `erased` is the one kind that crosses the boundary unreported. + expect(kindOf('export const Subject = { a: 1 };\nexport type Subject = typeof Subject;')).toBe('value'); + expect(kindOf('export function Subject() { return {}; }\nexport interface Subject { x: number }')).toBe('value'); + expect(kindOf('export class Subject extends Error {}\nexport interface Subject { x: number }')).toBe('value'); + // A declared namespace merges onto a function the same way. + expect( + kindOf('export function Subject() { return {}; }\nexport declare namespace Subject { const a: number; }') + ).toBe('value'); + }); + + it('still erases a type that shares its name with nothing', () => { + // The guard is one-directional: it must not turn a type-only export into a runtime one. + expect(kindOf('export type Subject = { a: 1 };')).toBe('erased'); + expect(kindOf('export declare const Subject: object;')).toBe('erased'); + // The reverse order was always right, and stays right. + expect(kindOf('export type Subject = { a: 1 };\nexport const Subject = { a: 1 } as const;')).toBe('value'); + }); + + it('lets an explicit type-only re-export outrank a star', () => { + // Not `recordErased`: `export * from './x'` beside `export type { Foo } from './x'` is legal, + // and the explicit clause is the one TypeScript honours. + const origin = new Map([['Foo', 'component']]); + const kinds = exportKindsOf( + parse('barrel.tsx', "export * from './x';\nexport type { Foo } from './x';"), + () => origin + ); + + expect(kinds.get('Foo')).toBe('erased'); + }); + it("does not attribute a nested scope's return to the component containing it", () => { // A method on a nested class is a scope of its own; its `return {}` is not the component's. expect( From 1c19dad9e3088e19d7fc84c0d26e7b068b8005a4 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Thu, 24 Sep 2026 17:46:49 -0700 Subject: [PATCH 18/23] test: let every other export outrank `export *`, and seed the four missing entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `export { Foo } from './values'` beside `export * from './components'` resolves `Foo` to `./values`, whichever order the two are written in — that is the language's rule, not a preference. Stars were applied in source order, so an explicit export written above one read as whatever the star happened to hold: a component, and waved through. The reverse order worked by accident. Stars now run in a second pass and fill only the names nothing else claimed, so a declaration, a named re-export, a namespace re-export and a type-only clause all outrank them from either side. This is the hole I described in the last round and left as a follow-up; it is a silent one, so it belongs here. `SERVER_ENTRY` was also short by four conventions, read off `FILE_TYPES` and `HTTP_ACCESS_FALLBACKS` in the installed Next 16.2.0 rather than from memory: `global-error`, `global-not-found`, `forbidden`, `unauthorized`. Next loads all four itself, so nothing has to import them, and a convention missing from the seed list is a subtree the walk never enters. `app/global-error.tsx` is in this tree today — it says `use client`, so it is excluded as a client module either way and the graph is unmoved at 407, which is why the pattern is tested directly rather than through the corpus. Eight mutations, eight failures. --- ...client-values-in-server-boundaries.test.ts | 152 +++++++++++++----- 1 file changed, 110 insertions(+), 42 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 463da39268..33d7a9162a 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -70,9 +70,15 @@ const SOURCE_DIRS = ['app', 'atoms', 'core', 'design-system', 'partials']; * The metadata routes belong here as much as the pages do — they are modules Next executes — and * `app/robots.ts` is already in this tree, so a client value reachable only through it was outside * the walk entirely. + * + * The list is Next's, read off `FILE_TYPES` and `HTTP_ACCESS_FALLBACKS` in the installed 16.2.0 + * rather than from memory. It was short by four: `global-error`, `global-not-found`, `forbidden` and + * `unauthorized`. Next loads all four by convention, so none of them is necessarily imported from a + * layout or a page, and a missing convention is a subtree the walk never visits — the quiet kind of + * gap. `app/global-error.tsx` is in this tree right now. */ const SERVER_ENTRY = - /^app\/(?:.*\/)?(layout|page|template|default|loading|error|not-found|route|opengraph-image|robots|sitemap|manifest|icon|apple-icon|twitter-image)\.tsx?$/; + /^app\/(?:.*\/)?(layout|page|template|default|loading|error|global-error|not-found|global-not-found|forbidden|unauthorized|route|opengraph-image|robots|sitemap|manifest|icon|apple-icon|twitter-image)\.tsx?$/; /** * What a module pulls in through a call rather than a declaration: `import('…')` and `require('…')`. @@ -635,6 +641,8 @@ function exportKindsOf( const recordErased = (name: string) => { if (!kinds.has(name)) kinds.set(name, 'erased'); }; + /** `export * from` specifiers, applied after every other export has had its say. */ + const stars: ts.StringLiteral[] = []; const react = reactBindingsOf(sourceFile); /** Local declarations, so an export by identifier has something to resolve against. */ const locals = new Map(); @@ -769,30 +777,10 @@ function exportKindsOf( continue; } - /* - * `export * from './x'` hands on every named export of `./x` — but never its default, which is - * the one binding `export *` does not carry. Without this a client barrel written that way had - * no exports at all as far as this was concerned, so everything taken from it came back - * unclassifiable. - */ + // `export * from './x'` is held back to a second pass below, because every other kind of export + // outranks it whatever order they are written in. if (ts.isExportDeclaration(statement) && !statement.exportClause && statement.moduleSpecifier) { - if (statement.isTypeOnly || !ts.isStringLiteral(statement.moduleSpecifier)) continue; - const origin = resolveOrigin(statement.moduleSpecifier.text); - - /* - * An unresolved target is not an empty one. Dropping it left the map with no bindings at all, - * so a server barrel star-re-exporting this one checked nothing and passed in silence — the - * worst shape of answer this guard can give. `*` is recorded instead, which reads as - * `re-exports *` and is reported. - */ - if (!origin) { - kinds.set('*', 'unknown'); - continue; - } - - for (const [name, kind] of origin) { - if (name !== 'default') kinds.set(name, kind); - } + if (!statement.isTypeOnly && ts.isStringLiteral(statement.moduleSpecifier)) stars.push(statement.moduleSpecifier); continue; } @@ -822,11 +810,11 @@ function exportKindsOf( for (const element of statement.exportClause.elements) { if (statement.isTypeOnly || element.isTypeOnly) { - // A plain `set`, not `recordErased`: an explicit clause outranks an `export *`, and - // `export * from './x'` beside `export type { Foo } from './x'` is legal, so this has to - // be able to overwrite what the star recorded. It cannot collide with a declaration in - // this module the way the two branches above can — `export const Foo` next to - // `export type { Foo }` is a duplicate export and does not compile. + // A plain `set`, not `recordErased`. This is an explicit claim on the name, and a claim is + // what the star pass below looks for — it has to land whether or not a star mentions the + // same name. It cannot collide with a declaration in this module the way the two branches + // above can: `export const Foo` next to `export type { Foo }` is a duplicate export and + // does not compile. kinds.set(element.name.text, 'erased'); continue; } @@ -855,6 +843,39 @@ function exportKindsOf( } } + /* + * `export * from './x'` hands on every named export of `./x` — but never its default, which is the + * one binding `export *` does not carry. Without this a client barrel written that way had no + * exports at all as far as this was concerned, so everything taken from it came back + * unclassifiable. + * + * Applied last, and only to names nothing else claimed. Every other form of export outranks a + * star — a declaration here, a named re-export, a namespace re-export, a type-only clause — and + * that is the language's rule, not a preference: in `export { Foo } from './values'` beside + * `export * from './components'`, `Foo` is the one from `./values` however the two are ordered. + * Applying stars in source order made the answer depend on which came last, so an explicit value + * written above a star read as whatever the star happened to hold — a component, and waved + * through. + */ + for (const star of stars) { + const origin = resolveOrigin(star.text); + + /* + * An unresolved target is not an empty one. Dropping it left the map with no bindings at all, so + * a server barrel star-re-exporting this one checked nothing and passed in silence — the worst + * shape of answer this guard can give. `*` is recorded instead, which reads as `re-exports *` + * and is reported. No export clause can claim that name, so nothing here can have taken it. + */ + if (!origin) { + kinds.set('*', 'unknown'); + continue; + } + + for (const [name, kind] of origin) { + if (name !== 'default' && !kinds.has(name)) kinds.set(name, kind); + } + } + return kinds; } @@ -1162,16 +1183,26 @@ describe('SERVER_ENTRY', () => { 'app/bounties/loading.tsx', 'app/robots.ts', 'app/api/chat/route.ts', + // Next loads these four by convention, so nothing has to import them. This one is in the tree. + 'app/global-error.tsx', + 'app/global-not-found.tsx', + 'app/space/[id]/forbidden.tsx', + 'app/space/[id]/unauthorized.tsx', ])('seeds %s', file => { expect(SERVER_ENTRY.test(file)).toBe(true); }); - it.each(['core/error.ts', 'partials/loading.tsx', 'design-system/page.tsx', 'atoms/route.ts'])( - 'does not seed %s', - file => { - expect(SERVER_ENTRY.test(file)).toBe(false); - } - ); + it.each([ + 'core/error.ts', + 'partials/loading.tsx', + 'design-system/page.tsx', + 'atoms/route.ts', + // The basename has to *be* the convention, not contain it. + 'app/lib/global-error-boundary.tsx', + 'app/lib/unauthorized-banner.tsx', + ])('does not seed %s', file => { + expect(SERVER_ENTRY.test(file)).toBe(false); + }); }); describe('staticReferences', () => { @@ -1572,18 +1603,55 @@ describe('exportKindsOf', () => { expect(kindOf('export type Subject = { a: 1 };\nexport const Subject = { a: 1 } as const;')).toBe('value'); }); - it('lets an explicit type-only re-export outrank a star', () => { - // Not `recordErased`: `export * from './x'` beside `export type { Foo } from './x'` is legal, - // and the explicit clause is the one TypeScript honours. + it.each([ + ['above', "export type { Foo } from './x';\nexport * from './x';"], + ['below', "export * from './x';\nexport type { Foo } from './x';"], + ])('lets an explicit type-only re-export written %s a star outrank it', (_label, source) => { + // Both orders compile, and the explicit clause is the one TypeScript honours in each. const origin = new Map([['Foo', 'component']]); - const kinds = exportKindsOf( - parse('barrel.tsx', "export * from './x';\nexport type { Foo } from './x';"), - () => origin - ); + const kinds = exportKindsOf(parse('barrel.tsx', source), () => origin); expect(kinds.get('Foo')).toBe('erased'); }); + it.each([ + ['above', "export { Foo } from './values';\nexport * from './components';"], + ['below', "export * from './components';\nexport { Foo } from './values';"], + ])('lets an explicit re-export written %s a star win', (_label, source) => { + // The language's rule, not a preference: `Foo` is the one from `./values` either way. Applying + // stars in source order meant an explicit value above a star read as the star's component and + // was waved through. + const origins = new Map([ + ['./values', new Map([['Foo', 'value']])], + ['./components', new Map([['Foo', 'component']])], + ]); + const kinds = exportKindsOf(parse('barrel.tsx', source), specifier => origins.get(specifier) ?? null); + + expect(kinds.get('Foo')).toBe('value'); + }); + + it.each([ + ['above', "export const Foo = { a: 1 };\nexport * from './components';"], + ['below', "export * from './components';\nexport const Foo = { a: 1 };"], + ])('lets a declaration written %s a star win', (_label, source) => { + const origin = new Map([['Foo', 'component']]); + const kinds = exportKindsOf(parse('barrel.tsx', source), () => origin); + + expect(kinds.get('Foo')).toBe('value'); + }); + + it('still takes a name only a star supplies', () => { + // The guard fills gaps; it must not stop a star being the source of a binding. + const origin = new Map([ + ['Foo', 'component'], + ['BAR', 'value'], + ]); + const kinds = exportKindsOf(parse('barrel.tsx', "export * from './x';"), () => origin); + + expect(kinds.get('Foo')).toBe('component'); + expect(kinds.get('BAR')).toBe('value'); + }); + it("does not attribute a nested scope's return to the component containing it", () => { // A method on a nested class is a scope of its own; its `return {}` is not the component's. expect( From 1c74ec2bfe289135e699e5a3c596c7a364b59189 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Thu, 24 Sep 2026 18:21:37 -0700 Subject: [PATCH 19/23] test: report a read this cannot name, and stop accusing a type-only star MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two directions of the same mistake — a reference the walk could not resolve to one export was thrown away, and a name the walk never recorded was treated as suspicious. `require('./client')[key]` definitely reads an export and cannot say which. That is the position a namespace binding is in, and a namespace is reported, but this became a bare edge instead — and `clientBindings` skips a reference with neither a name nor `namespace`, so the read was invisible. It now reports, along with the two destructuring forms of the same thing: `{ [key]: v }`, and `{ ...rest }`, which takes every export not named above it and was claiming an export literally called `rest`. An array destructure stays a bare edge, because `x[0]` is not a named export of anything, and `const {} = require(…)` gets its edge back — it binds nothing and still loads the module, the way `import {}` does, and returning no reference at all dropped the traversal with it. `export type * from './x'` was skipped outright, so every name it carries was missing from the map — and a name this cannot find reads as `unknown`, which is reported. A client barrel written that way had its erased bindings accused of being client values. Its names are now recorded as erased, and `export type * as Ns` with them, which was the same omission one branch over. An unresolved type-only star records no wildcard sentinel: that sentinel exists because an unresolved star might be hiding a client value, and a type-only one carries nothing at runtime. `core/utils/diff/index.ts` is the tree's one `export type *`. It is a server module, so nothing moves today: 407 modules, the same five offences. Nine mutations, nine failures, one test each. --- ...client-values-in-server-boundaries.test.ts | 119 ++++++++++++++++-- 1 file changed, 107 insertions(+), 12 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 33d7a9162a..030362540e 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -136,8 +136,11 @@ function requireBindings(call: ts.CallExpression, specifier: string): CallRefere (ts.isPropertyAccessExpression(parent) || ts.isElementAccessExpression(parent)) && parent.expression === call ) { + // `require('./x')[key]` definitely reads an export and cannot say which, which is the position a + // namespace binding is in — so it is reported the same way rather than dropped. As a bare edge it + // was invisible: `clientBindings` skips a reference with neither a name nor `namespace`. const exported = accessedName(parent); - if (!exported) return [{ specifier, local: '' }]; + if (!exported) return [{ specifier, local: '[computed]', namespace: true }]; const assignedTo = parent.parent; const local = @@ -154,13 +157,28 @@ function requireBindings(call: ts.CallExpression, specifier: string): CallRefere const bindings: CallReference[] = []; for (const element of parent.name.elements) { + // `{ ...rest }` takes every export not named above it, and `{ [key]: v }` takes one nobody + // can name here. Both are the computed case in destructuring form. + if (element.dotDotDotToken) { + bindings.push({ specifier, local: '{ ...rest }', namespace: true }); + continue; + } + const key = element.propertyName ?? element.name; const exported = ts.isIdentifier(key) || ts.isStringLiteralLike(key) ? key.text : null; const local = ts.isIdentifier(element.name) ? element.name.text : exported; - if (exported && local) bindings.push({ specifier, exported, local }); + + if (!exported || !local) { + bindings.push({ specifier, local: '{ [computed] }', namespace: true }); + continue; + } + + bindings.push({ specifier, exported, local }); } - return bindings; + // `const {} = require('./x')` binds nothing and still loads the module, the same way + // `import {} from './x'` does. Returning no reference at all dropped the edge with it. + return bindings.length > 0 ? bindings : [{ specifier, local: '' }]; } // `const ns = require('./x')` keeps the whole module object. @@ -188,7 +206,11 @@ type Reference = { specifier: string; exported?: string; local: string; - /** `import * as X` / `export * as X`: an object whose every property is a client reference. */ + /** + * A binding this cannot pin to one export, so every export is in play: `import * as X`, + * `export * as X`, and a `require` read under a name only known at runtime. Each is reported, + * because any property of one belonging to a client module is a client reference. + */ namespace?: boolean; /** `export * from`: the target's named bindings, each classified on its own. */ starReexport?: boolean; @@ -642,7 +664,7 @@ function exportKindsOf( if (!kinds.has(name)) kinds.set(name, 'erased'); }; /** `export * from` specifiers, applied after every other export has had its say. */ - const stars: ts.StringLiteral[] = []; + const stars: { specifier: ts.StringLiteral; typeOnly: boolean }[] = []; const react = reactBindingsOf(sourceFile); /** Local declarations, so an export by identifier has something to resolve against. */ const locals = new Map(); @@ -780,7 +802,9 @@ function exportKindsOf( // `export * from './x'` is held back to a second pass below, because every other kind of export // outranks it whatever order they are written in. if (ts.isExportDeclaration(statement) && !statement.exportClause && statement.moduleSpecifier) { - if (!statement.isTypeOnly && ts.isStringLiteral(statement.moduleSpecifier)) stars.push(statement.moduleSpecifier); + if (ts.isStringLiteral(statement.moduleSpecifier)) { + stars.push({ specifier: statement.moduleSpecifier, typeOnly: statement.isTypeOnly }); + } continue; } @@ -788,7 +812,10 @@ function exportKindsOf( // property read off one belonging to a client module is a client reference — and recorded at all, // because a barrel over this barrel would otherwise lose the binding entirely. if (ts.isExportDeclaration(statement) && statement.exportClause && ts.isNamespaceExport(statement.exportClause)) { - if (!statement.isTypeOnly) kinds.set(statement.exportClause.name.text, 'value'); + // `export type * as Ns` is the same object with nothing of it left at runtime. Skipping it + // left the name out of the map entirely, and a name this cannot find reads as `unknown` — + // which is an offence, so an erased binding was accused of being a client value. + kinds.set(statement.exportClause.name.text, statement.isTypeOnly ? 'erased' : 'value'); continue; } @@ -857,8 +884,8 @@ function exportKindsOf( * written above a star read as whatever the star happened to hold — a component, and waved * through. */ - for (const star of stars) { - const origin = resolveOrigin(star.text); + for (const { specifier, typeOnly } of stars) { + const origin = resolveOrigin(specifier.text); /* * An unresolved target is not an empty one. Dropping it left the map with no bindings at all, so @@ -867,12 +894,20 @@ function exportKindsOf( * and is reported. No export clause can claim that name, so nothing here can have taken it. */ if (!origin) { - kinds.set('*', 'unknown'); + // A type-only star carries nothing at runtime, so an unresolved one hides no client value and + // the sentinel would be an offence invented out of a type import. + if (!typeOnly) kinds.set('*', 'unknown'); continue; } + /* + * `export type * from './x'` re-exports the same names with nothing of them left at runtime, and + * this tree has one — `core/utils/diff/index.ts`. Skipping the statement left every name it + * carries out of the map, and a name this cannot find reads as `unknown`, which is reported. So + * a client barrel written that way had its erased bindings accused of being client values. + */ for (const [name, kind] of origin) { - if (name !== 'default' && !kinds.has(name)) kinds.set(name, kind); + if (name !== 'default' && !kinds.has(name)) kinds.set(name, typeOnly ? 'erased' : kind); } } @@ -1335,11 +1370,33 @@ describe('callReferences', () => { it.each([ ['a call for its side effects', "require('./x');"], - ['a computed property read', "const thing = require('./x')[key];"], + // Binds nothing and still loads the module, the same way `import {} from './x'` does. + ['a destructure that binds nothing', "const {} = require('./x');"], + // Reads `x[0]`, which is not a named export of anything. + ['an array destructure', "const [first] = require('./x');"], ])('takes no binding from %s, but still walks the module', (_label, source) => { expect(refs(source)).toEqual([{ specifier: './x', local: '' }]); }); + it.each([ + ['a computed property read', "const thing = require('./x')[key];", '[computed]'], + ['a computed key in a destructure', "const { [key]: thing } = require('./x');", '{ [computed] }'], + ])('reports %s, which it cannot pin to one export', (_label, source, local) => { + // Each of these definitely reads an export and cannot say which — the position a namespace + // binding is in, so reported the same way. As bare edges they were invisible: `clientBindings` + // skips a reference with neither a name nor `namespace`. + expect(refs(source)).toEqual([{ specifier: './x', local, namespace: true }]); + }); + + it('reports a rest element alongside the names taken above it', () => { + // `...rest` takes every export not named before it, so the named ones stay resolved and the rest + // is the unpinnable read. + expect(refs("const { A, ...rest } = require('./x');")).toEqual([ + { specifier: './x', exported: 'A', local: 'A' }, + { specifier: './x', local: '{ ...rest }', namespace: true }, + ]); + }); + it.each([ ['export default', "export default require('./x');", 'export default'], ['export =', "export = require('./x');", 'export ='], @@ -1652,6 +1709,44 @@ describe('exportKindsOf', () => { expect(kinds.get('BAR')).toBe('value'); }); + it('keeps the names a type-only star carries, as erased', () => { + // `export type * from './types'` is in this tree at `core/utils/diff/index.ts`. Skipping the + // statement left its names out of the map, and a name this cannot find reads as `unknown`, + // which is an offence — so a client barrel written that way was accused over a type import. + const origin = new Map([ + ['Change', 'value'], + ['ChangeKind', 'erased'], + ]); + const kinds = exportKindsOf(parse('barrel.tsx', "export type * from './types';"), () => origin); + + expect(kinds.get('Change')).toBe('erased'); + expect(kinds.get('ChangeKind')).toBe('erased'); + }); + + it('lets a declaration outrank a type-only star, like any other', () => { + const origin = new Map([['Change', 'value']]); + const kinds = exportKindsOf( + parse('barrel.tsx', "export const Change = { a: 1 };\nexport type * from './types';"), + () => origin + ); + + expect(kinds.get('Change')).toBe('value'); + }); + + it('records a type-only namespace re-export as erased rather than dropping it', () => { + const kinds = exportKindsOf(parse('barrel.tsx', "export type * as Ns from './x';"), () => null); + + expect(kinds.get('Ns')).toBe('erased'); + }); + + it('does not invent a wildcard offence out of an unresolved type-only star', () => { + // The value-star sentinel exists because an unresolved star could be hiding a client value. A + // type-only one carries nothing at runtime, so there is nothing for it to hide. + const kinds = exportKindsOf(parse('barrel.tsx', "export type * from './missing';"), () => null); + + expect(kinds.has('*')).toBe(false); + }); + it("does not attribute a nested scope's return to the component containing it", () => { // A method on a nested class is a scope of its own; its `return {}` is not the component's. expect( From 6b0b4e11055bce5833f4b3b4b5e25b646483f39e Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Fri, 25 Sep 2026 11:18:26 -0700 Subject: [PATCH 20/23] test: let a barrel's own exports shadow the star beside them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An explicit export shadows `export * from` whichever order the two are written in, so `export { restFetch } from './client'` beside `export * from './schemas'` does not re-export a `restFetch` from `./schemas` even if one is there. The star was expanded to every name its target has, so the walk could report a binding the barrel does not have — the accusing direction, and a guard that accuses is one somebody deletes. `claimedExportNames` is the set a star cannot fill: named and namespace re-exports, local declarations, and types, which claim a name as firmly as values do. `starReexports` subtracts it, and subtracts the default the star never carried — a rule that was an inline `continue` in the suite and is now somewhere a fixture can reach. I said twice in review that no module here mixes the two forms. That was wrong, and the scan I based it on was wrong: `core/io/rest/index.ts` and `atoms/index.ts` both do. Neither star targets a client module, so nothing moves today — 407 modules, the same five offences — but the reason I gave for deferring this did not hold. Ten mutations, ten failures. --- ...client-values-in-server-boundaries.test.ts | 149 +++++++++++++++++- 1 file changed, 143 insertions(+), 6 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 030362540e..74de70d416 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -214,6 +214,11 @@ type Reference = { namespace?: boolean; /** `export * from`: the target's named bindings, each classified on its own. */ starReexport?: boolean; + /** + * With `starReexport`: the names this module exports itself, which a star never gets to supply. + * Without them the walk reported bindings the barrel does not have. + */ + claimed?: ReadonlySet; reexported?: boolean; }; @@ -230,8 +235,60 @@ type Reference = { * left the corpus green and reopened the gap it was added to close, which is the same hole the * `callReferences` fixtures were written for. */ +/** + * Every export name a module states for itself, which is every name a star cannot supply. + * + * An explicit export shadows `export * from` whatever order the two are written in, so a barrel + * doing `export { restFetch } from './client'` beside `export * from './schemas'` does not re-export + * a `restFetch` from `./schemas` even if one is there. `core/io/rest/index.ts` and `atoms/index.ts` + * are both written that way. + * + * `exportKindsOf` needs the same fact and does not call this: by the time its star pass runs, the + * keys of its own map *are* the claimed names, so asking twice would be two ways to be wrong. + */ +function claimedExportNames(sourceFile: ts.SourceFile): Set { + const claimed = new Set(); + + for (const statement of sourceFile.statements) { + if (ts.isExportDeclaration(statement)) { + // A bare star claims nothing of its own; that is the whole point of it. + if (!statement.exportClause) continue; + if (ts.isNamespaceExport(statement.exportClause)) claimed.add(statement.exportClause.name.text); + else for (const element of statement.exportClause.elements) claimed.add(element.name.text); + continue; + } + + // A type or an interface claims the name as firmly as a value does: the star still cannot fill it. + if (!isExported(statement)) continue; + + if (ts.isVariableStatement(statement)) { + for (const declaration of statement.declarationList.declarations) { + if (ts.isIdentifier(declaration.name)) claimed.add(declaration.name.text); + } + continue; + } + + const named = (statement as ts.DeclarationStatement).name; + if (named && ts.isIdentifier(named)) claimed.add(named.text); + } + + return claimed; +} + +/** + * What a barrel actually hands on through `export * from './x'`. + * + * The target's named exports, minus its default — which a star does not carry, so reading one here + * invents an offence — and minus every name the barrel claims itself. + */ +function starReexports(targetKinds: Map, claimed: ReadonlySet): [string, ExportKind][] { + return [...targetKinds].filter(([name]) => name !== 'default' && !claimed.has(name)); +} + function staticReferences(sourceFile: ts.SourceFile): Reference[] { const found: Reference[] = []; + // Computed at most once, and only for a module that actually has a star to shadow. + let claimed: Set | null = null; for (const statement of sourceFile.statements) { if (ts.isImportDeclaration(statement) && ts.isStringLiteral(statement.moduleSpecifier)) { @@ -287,7 +344,8 @@ function staticReferences(sourceFile: ts.SourceFile): Reference[] { // Not a namespace value: this hands on the target's named bindings one by one, so a // barrel star-re-exporting nothing but components is not an offence. `export * as Ns` is // a namespace object and stays one, below. - found.push({ specifier, local: 're-exports *', starReexport: true, reexported: true }); + claimed ??= claimedExportNames(sourceFile); + found.push({ specifier, local: 're-exports *', starReexport: true, reexported: true, claimed }); } else if (ts.isNamespaceExport(statement.exportClause)) { found.push({ specifier, @@ -1035,10 +1093,7 @@ describe('server components take only components from client modules', () => { // `export * from` is every named binding the target has, so each is judged on its own. if (reference.starReexport) { - for (const [exported, kind] of kindsFor(target)) { - // `export *` does not carry the default export, so reading one here invents an offence. - if (exported === 'default') continue; - + for (const [exported, kind] of starReexports(kindsFor(target), reference.claimed ?? new Set())) { // One name here: a re-export has no local binding in this file to read. const verdict = verdictFor([exported], kind); @@ -1240,6 +1295,73 @@ describe('SERVER_ENTRY', () => { }); }); +describe('claimedExportNames', () => { + const claimed = (source: string) => [...claimedExportNames(parse('barrel.tsx', source))].sort(); + + it.each([ + ['a named re-export', "export { Foo } from './x';"], + ['a named re-export under an alias', "export { Inner as Foo } from './x';"], + ['a local re-export', 'const Foo = 1;\nexport { Foo };'], + ['a namespace re-export', "export * as Foo from './x';"], + ['a const', 'export const Foo = { a: 1 };'], + ['a function', 'export function Foo() { return {}; }'], + ['a class', 'export class Foo {}'], + ['an enum', 'export enum Foo { A }'], + ['a namespace', 'export namespace Foo { export const a = 1; }'], + // A type claims the name as firmly as a value: a star still cannot fill it. + ['a type alias', 'export type Foo = { a: 1 };'], + ['an interface', 'export interface Foo { a: 1 }'], + ['a type-only re-export', "export type { Foo } from './x';"], + ])('counts %s', (_label, source) => { + expect(claimed(source)).toEqual(['Foo']); + }); + + it.each([ + // The point of a bare star is that it claims nothing of its own. + ['a bare star', "export * from './x';"], + ['a declaration that is not exported', 'const Foo = 1;'], + ['an import', "import { Foo } from './x';"], + ])('does not count %s', (_label, source) => { + expect(claimed(source)).toEqual([]); + }); + + it('reads the aliased name, not the origin one', () => { + // `export { Inner as Foo }` claims `Foo`. Claiming `Inner` would leave `Foo` open to a star and + // shadow a name the barrel never mentions. + expect(claimed("export { Inner as Foo } from './x';")).toEqual(['Foo']); + }); +}); + +describe('starReexports', () => { + const kinds = new Map([ + ['Button', 'component'], + ['BUTTON_CLASS', 'value'], + ['default', 'value'], + ]); + + it('hands on the target named exports', () => { + expect(starReexports(kinds, new Set())).toEqual([ + ['Button', 'component'], + ['BUTTON_CLASS', 'value'], + ]); + }); + + it('never hands on a default, which a star does not carry', () => { + // Reading one here invents an offence against a binding nothing can import. + expect(starReexports(kinds, new Set()).map(([name]) => name)).not.toContain('default'); + }); + + it('leaves out a name the barrel claims itself', () => { + // An explicit export shadows a star whichever order the two are written in, so the star is not + // where this name comes from and an offence against it is against a binding that does not exist. + expect(starReexports(kinds, new Set(['BUTTON_CLASS']))).toEqual([['Button', 'component']]); + }); + + it('hands on nothing when the barrel claims everything', () => { + expect(starReexports(kinds, new Set(['Button', 'BUTTON_CLASS']))).toEqual([]); + }); +}); + describe('staticReferences', () => { const refs = (source: string) => staticReferences(parse('fixture.tsx', source)); @@ -1282,13 +1404,28 @@ describe('staticReferences', () => { // `export *` hands on named bindings one by one, so a barrel of components is not an offence. // `export * as Ns` is a namespace object, and every property read off one is a client reference. expect(refs("export * from './x';")).toEqual([ - { specifier: './x', local: 're-exports *', starReexport: true, reexported: true }, + { specifier: './x', local: 're-exports *', starReexport: true, reexported: true, claimed: new Set() }, ]); expect(refs("export * as Ns from './x';")).toEqual([ { specifier: './x', local: 're-exports * as Ns', namespace: true, reexported: true }, ]); }); + it('tells a star what the barrel around it already exports', () => { + // `core/io/rest/index.ts` is written this way. Without the claimed set the walk reported a + // `restFetch` coming from `./schemas`, which this module does not re-export from there. + expect(refs("export { restFetch } from './client';\nexport * from './schemas';")).toEqual([ + { specifier: './client', exported: 'restFetch', local: 'restFetch', reexported: true }, + { + specifier: './schemas', + local: 're-exports *', + starReexport: true, + reexported: true, + claimed: new Set(['restFetch']), + }, + ]); + }); + it('reads call edges alongside the declared ones', () => { expect(refs("import './a';\nvoid import('./b');")).toEqual([ { specifier: './a', local: '' }, From bab48d271b31cc57e4ecdbc465c23b44d5069854 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Fri, 25 Sep 2026 11:53:00 -0700 Subject: [PATCH 21/23] test: claim destructured exports, merge namespaces, report escaped requires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `export const { Foo } = values` exports `Foo`, and both the claim set and the kind map read only plain identifiers, so the name was unclaimed and a star beside it filled the slot with its own `Foo` — a component, with the real value waved through. Every name a binding pattern binds is now claimed and recorded as unknown, which is the honest answer and is reported. A namespace merged onto a function or class declared above it leaves the binding that function or class, but the namespace branch wrote `value` over it and so accused a real component. It now only fills an empty slot, and a namespace that holds nothing but types is erased like a type alias, because it builds nothing. The `require` fallback treated everything it did not recognise as a side-effect load. Only a discarded result is one; a module object that escapes — passed to a function, picked by a ternary, called, or array-destructured — is a read of the whole client module, and is now reported the way a namespace binding is. The array case reverses a fixture I committed last round. None of these shapes occurs in the tree: 407 modules, the same five offences. Twelve mutations, twelve failures. --- ...client-values-in-server-boundaries.test.ts | 152 +++++++++++++++++- 1 file changed, 144 insertions(+), 8 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 74de70d416..5c453b2316 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -185,6 +185,10 @@ function requireBindings(call: ts.CallExpression, specifier: string): CallRefere if (ts.isIdentifier(parent.name)) { return [{ specifier, local: `* as ${parent.name.text}`, namespace: true }]; } + + // `const [first] = require('./x')` reads `Symbol.iterator` off the module object. No named + // export, but a read of the client module all the same. + return [{ specifier, local: '[ … ]', namespace: true }]; } /* @@ -197,8 +201,20 @@ function requireBindings(call: ts.CallExpression, specifier: string): CallRefere return [{ specifier, local: parent.isExportEquals ? 'export =' : 'export default', namespace: true }]; } - // Anything else — a bare call for its side effects — loads the module and takes nothing. - return [{ specifier, local: '' }]; + /* + * Only a result that is thrown away is a side-effect load: `require('./x');`, parenthesised or + * `void`ed. This fallback used to assume that of everything it did not recognise, so a module + * object passed to a function, picked by a ternary or called outright was recorded as taking + * nothing — and `clientBindings` skips a reference that takes nothing. Whatever escapes is the + * whole module object, which is the position a namespace binding is in. + */ + let outer: ts.Node = call; + while (outer.parent && ts.isParenthesizedExpression(outer.parent)) outer = outer.parent; + if (outer.parent && (ts.isExpressionStatement(outer.parent) || ts.isVoidExpression(outer.parent))) { + return [{ specifier, local: '' }]; + } + + return [{ specifier, local: 'require(…)', namespace: true }]; } /** One binding taken from another module: a named export, a default, or a whole namespace. */ @@ -235,6 +251,38 @@ type Reference = { * left the corpus green and reopened the gap it was added to close, which is the same hole the * `callReferences` fixtures were written for. */ +/** + * Whether a namespace builds anything at runtime. One holding only types, interfaces, type-only + * imports and exports, ambient declarations, or other namespaces like it compiles to nothing. + * + * A `const enum` inside one counts as instantiated. Whether it survives depends on + * `preserveConstEnums`, and guessing wrong in this direction reports rather than hides. + */ +function isInstantiated(node: ts.ModuleDeclaration): boolean { + const body = node.body; + if (!body) return false; + // `namespace A.B {}` nests the second name as the body of the first. + if (ts.isModuleDeclaration(body)) return isInstantiated(body); + if (!ts.isModuleBlock(body)) return false; + + return body.statements.some(statement => { + if (ts.isInterfaceDeclaration(statement) || ts.isTypeAliasDeclaration(statement)) return false; + if (ts.isModuleDeclaration(statement)) return isInstantiated(statement); + if (ts.isImportDeclaration(statement) && statement.importClause?.isTypeOnly) return false; + if (ts.isExportDeclaration(statement) && statement.isTypeOnly) return false; + return !hasModifier(statement, ts.SyntaxKind.DeclareKeyword); + }); +} + +/** + * Every name a declaration binds. `export const { a, b: { c }, ...rest } = values` exports `a`, + * `c` and `rest`; reading only a plain identifier exported none of them. + */ +function bindingNames(name: ts.BindingName): string[] { + if (ts.isIdentifier(name)) return [name.text]; + return name.elements.flatMap(element => (ts.isOmittedExpression(element) ? [] : bindingNames(element.name))); +} + /** * Every export name a module states for itself, which is every name a star cannot supply. * @@ -263,7 +311,7 @@ function claimedExportNames(sourceFile: ts.SourceFile): Set { if (ts.isVariableStatement(statement)) { for (const declaration of statement.declarationList.declarations) { - if (ts.isIdentifier(declaration.name)) claimed.add(declaration.name.text); + for (const name of bindingNames(declaration.name)) claimed.add(name); } continue; } @@ -825,9 +873,17 @@ function exportKindsOf( continue; } - // `export namespace Foo {}` builds an object at runtime, so it is a value. + /* + * `export namespace Foo { export const a = 1 }` builds an object at runtime, so it is a value — + * unless it merges onto a function or class declared above it, which is legal and leaves the + * binding that function or class: `export function Card() {…}` then `export namespace Card {…}` + * is still a component with properties on it, and writing `value` over it accused one. And a + * namespace holding only types builds nothing at all, so it is erased like a type alias. + */ if (ts.isModuleDeclaration(statement) && isExported(statement) && ts.isIdentifier(statement.name)) { - kinds.set(statement.name.text, 'value'); + const name = statement.name.text; + if (!isInstantiated(statement)) recordErased(name); + else if (!kinds.has(name) || kinds.get(name) === 'erased') kinds.set(name, 'value'); continue; } @@ -845,7 +901,16 @@ function exportKindsOf( if (ts.isVariableStatement(statement) && isExported(statement)) { for (const declaration of statement.declarationList.declarations) { - if (!ts.isIdentifier(declaration.name)) continue; + /* + * `export const { Foo } = values` exports `Foo` and says nothing about what it is. Skipping it + * left the name unclaimed, so a star beside it filled the slot with whatever *its* `Foo` was — + * a component, and the real value waved through. Unknown is the honest answer, and it is + * reported. + */ + if (!ts.isIdentifier(declaration.name)) { + for (const name of bindingNames(declaration.name)) kinds.set(name, 'unknown'); + continue; + } kinds.set(declaration.name.text, declaration.initializer ? classify(declaration.initializer) : 'unknown'); } continue; @@ -1325,6 +1390,15 @@ describe('claimedExportNames', () => { expect(claimed(source)).toEqual([]); }); + it.each([ + ['an object pattern', 'export const { Foo } = values;', ['Foo']], + ['a nested pattern with a rest', 'export const { a: { Foo }, ...Rest } = values;', ['Foo', 'Rest']], + ['an array pattern with a hole', 'export const [Foo, , Bar] = values;', ['Bar', 'Foo']], + ])('counts every name %s binds', (_label, source, names) => { + // Reading only a plain identifier claimed none of these, so a star could supply any of them. + expect(claimed(source)).toEqual(names); + }); + it('reads the aliased name, not the origin one', () => { // `export { Inner as Foo }` claims `Foo`. Claiming `Inner` would leave `Foo` open to a star and // shadow a name the barrel never mentions. @@ -1507,14 +1581,27 @@ describe('callReferences', () => { it.each([ ['a call for its side effects', "require('./x');"], + ['a parenthesised call for its side effects', "(require('./x'));"], + ['a voided call', "void require('./x');"], // Binds nothing and still loads the module, the same way `import {} from './x'` does. ['a destructure that binds nothing', "const {} = require('./x');"], - // Reads `x[0]`, which is not a named export of anything. - ['an array destructure', "const [first] = require('./x');"], ])('takes no binding from %s, but still walks the module', (_label, source) => { expect(refs(source)).toEqual([{ specifier: './x', local: '' }]); }); + it.each([ + // Reads `Symbol.iterator` off the module object. I had this as a bare edge on the grounds that + // `x[0]` names no export; the question here is whether the module object is read, and it is. + ['an array destructure', "const [first] = require('./x');", '[ … ]'], + ['a module passed to a function', "use(require('./x'));", 'require(…)'], + ['a module picked by a ternary', "const m = on ? require('./x') : null;", 'require(…)'], + ['a module called outright', "require('./x')();", 'require(…)'], + ])('reports %s as the whole module object escaping', (_label, source, local) => { + // Only a result thrown away is a side-effect load. These were all recorded as taking nothing, + // which `clientBindings` skips. + expect(refs(source)).toEqual([{ specifier: './x', local, namespace: true }]); + }); + it.each([ ['a computed property read', "const thing = require('./x')[key];", '[computed]'], ['a computed key in a destructure', "const { [key]: thing } = require('./x');", '{ [computed] }'], @@ -1733,6 +1820,55 @@ describe('exportKindsOf', () => { expect(kinds.get('*')).toBe('unknown'); }); + it.each([ + ['above', "export const { Foo } = values;\nexport * from './components';"], + ['below', "export * from './components';\nexport const { Foo } = values;"], + ])('does not let a star fill a name destructured %s it', (_label, source) => { + // `Foo` is this module's own, from `values`, and nothing here says what it is. The star used to + // fill it with its component and wave the real one through. + const origin = new Map([['Foo', 'component']]); + const kinds = exportKindsOf(parse('barrel.tsx', source), () => origin); + + expect(kinds.get('Foo')).toBe('unknown'); + }); + + it('records every name a destructured export binds', () => { + const kinds = exportKindsOf(parse('fixture.tsx', 'export const { a: { Foo }, ...Rest } = values;'), () => null); + + expect([kinds.get('Foo'), kinds.get('Rest')]).toEqual(['unknown', 'unknown']); + }); + + it.each([ + ['a function', 'export function Subject() { return
; }\nexport namespace Subject { export const a = 1; }'], + [ + 'a React class', + "import * as React from 'react';\nexport class Subject extends React.Component {}\nexport namespace Subject { export const a = 1; }", + ], + ])('keeps %s a component when a namespace merges onto it', (_label, source) => { + // Legal, and the binding is still the function or class — with properties on it. Writing + // `value` over it accused a real component. + expect(kindOf(source)).toBe('component'); + }); + + it.each([ + ['holding only types', 'export namespace Subject { export type T = 1; export interface U {} }'], + ['with nothing in it', 'export namespace Subject {}'], + ['nesting only types', 'export namespace Subject { export namespace Inner { export type T = 1; } }'], + ])('erases a namespace %s, which builds nothing', (_label, source) => { + expect(kindOf(source)).toBe('erased'); + }); + + it.each([ + ['dotted', 'export namespace Subject.Inner { export const a = 1; }'], + ['after a type of the same name', 'export interface Subject {}\nexport namespace Subject { export const a = 1; }'], + [ + 'after a type-only block', + 'export namespace Subject { export type T = 1; }\nexport namespace Subject { export const a = 1; }', + ], + ])('keeps a namespace that builds something a value: %s', (_label, source) => { + expect(kindOf(source)).toBe('value'); + }); + it('erases an ambient namespace and keeps a real one', () => { // `export namespace` builds an object at runtime; `declare` does not build anything. expect(kindOf('export declare namespace Subject { const a: number; }')).toBe('erased'); From 87f40ae7f1391f164b2ba993821bd6ca923291f6 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Fri, 25 Sep 2026 12:05:54 -0700 Subject: [PATCH 22/23] test: resolve require by scope, and fail on any dropped local import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every identifier spelled `require` was taken for the CommonJS loader, so `function load(require) { return require('./client'); }` invented an edge to `./client` and, had it said `use client`, an offence. `isShadowed` walks outward through the scopes that can bind a name — parameters, a function expression's own name, catch and for variables, and the declarations in each enclosing block — and only the ambient `require` is followed. `~` maps to the app root, so a server import of a root-level file such as `proxy.ts` resolved to nothing and the walk dropped it and everything behind it, silently: a missing edge looks exactly like a module that imports less. Review suggested loading root-level files too. That closes one location and leaves `prebundled/`, the excluded `scripts/` and any future directory open the same way, so instead a new assertion fails whenever a server module reaches source the walk does not load, and names the import. Planting `import '~/vercel'` in the root layout passes every test on the old guard and fails this one. Neither shape occurs in the tree: the only unresolved local imports anywhere are four stylesheets. 407 modules, the same five offences. --- ...client-values-in-server-boundaries.test.ts | 152 ++++++++++++++++-- 1 file changed, 140 insertions(+), 12 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 5c453b2316..80fe24aa6a 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -1,6 +1,6 @@ // This walks the source tree with `fs` and never touches the DOM. // @vitest-environment node -import { readFileSync, readdirSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync } from 'node:fs'; import path from 'node:path'; import ts from 'typescript'; import { describe, expect, it } from 'vitest'; @@ -97,7 +97,8 @@ function callReferences(sourceFile: ts.SourceFile): CallReference[] { const visit = (node: ts.Node) => { if (ts.isCallExpression(node)) { const isDynamicImport = node.expression.kind === ts.SyntaxKind.ImportKeyword; - const isRequire = ts.isIdentifier(node.expression) && node.expression.text === 'require'; + const isRequire = + ts.isIdentifier(node.expression) && node.expression.text === 'require' && !isShadowed(node.expression); const [specifier] = node.arguments; if ((isDynamicImport || isRequire) && specifier && ts.isStringLiteralLike(specifier)) { @@ -113,6 +114,60 @@ function callReferences(sourceFile: ts.SourceFile): CallReference[] { return found; } +/** + * Whether an identifier refers to something declared in scope rather than to the ambient global. + * + * `function load(require) { return require('./client'); }` calls its own parameter, and reading + * every `require` by spelling invented an edge to `./client` — and an offence, if that file says + * `use client`. This walks outward through the scopes that can bind a name: parameters, a function + * expression's own name, a `catch` variable, a `for` initialiser, and the declarations directly in + * a block, module or source file. It reads the tree, not a type checker, so it does not model + * `with` or `eval`, neither of which a module can use. + */ +function isShadowed(identifier: ts.Identifier): boolean { + const name = identifier.text; + const declaresIn = (statements: ts.NodeArray) => + statements.some(statement => { + if (ts.isVariableStatement(statement)) { + return statement.declarationList.declarations.some(d => bindingNames(d.name).includes(name)); + } + if ((ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) && statement.name) { + return statement.name.text === name; + } + if (ts.isImportDeclaration(statement) && statement.importClause) { + const clause = statement.importClause; + if (clause.name?.text === name) return true; + const bindings = clause.namedBindings; + if (bindings && ts.isNamespaceImport(bindings)) return bindings.name.text === name; + return Boolean(bindings?.elements.some(element => element.name.text === name)); + } + return false; + }); + + for (let scope: ts.Node | undefined = identifier.parent; scope; scope = scope.parent) { + if (ts.isFunctionLike(scope)) { + if (scope.parameters.some(parameter => bindingNames(parameter.name).includes(name))) return true; + // `const load = function require() {…}` binds its own name inside itself only. + if (ts.isFunctionExpression(scope) && scope.name?.text === name) return true; + } + if (ts.isCatchClause(scope) && scope.variableDeclaration) { + if (bindingNames(scope.variableDeclaration.name).includes(name)) return true; + } + if ( + (ts.isForStatement(scope) || ts.isForInStatement(scope) || ts.isForOfStatement(scope)) && + scope.initializer && + ts.isVariableDeclarationList(scope.initializer) && + scope.initializer.declarations.some(d => bindingNames(d.name).includes(name)) + ) { + return true; + } + if ((ts.isBlock(scope) || ts.isModuleBlock(scope) || ts.isSourceFile(scope)) && declaresIn(scope.statements)) { + return true; + } + } + return false; +} + /** What a `require()` call's surroundings say is being taken from it. */ type CallReference = { specifier: string; exported?: string; local: string; namespace?: boolean }; @@ -459,6 +514,19 @@ const KNOWN = new Set([ 'core/bounties/config.ts -> useFeatureFlag (from core/state/feature-flags.ts)', ]); +/** + * Where a local specifier points, relative to the app root, before extensions are tried. `null` for + * a package, which is not this walk's to follow. + */ +function localPath(specifier: string, importingFile: string): string | null { + if (specifier.startsWith('~/')) return path.normalize(specifier.slice(2)); + if (specifier.startsWith('.')) return path.join(path.dirname(importingFile), specifier); + return null; +} + +/** A file an import can load that is not source: a stylesheet, an image, a font. */ +const SOURCE_EXTENSION = /\.[cm]?[jt]sx?$/; + function sourceFiles(): string[] { const found: string[] = []; const walk = (dir: string) => { @@ -1067,17 +1135,9 @@ describe('server components take only components from client modules', () => { } function resolveImport(specifier: string, importingFile: string): string | null { - let absolute: string; - - if (specifier.startsWith('~/')) { - absolute = path.join(ROOT, specifier.slice(2)); - } else if (specifier.startsWith('.')) { - absolute = path.resolve(ROOT, path.dirname(importingFile), specifier); - } else { - return null; - } + const relative = localPath(specifier, importingFile); + if (relative === null) return null; - const relative = path.relative(ROOT, absolute); for (const candidate of [ `${relative}.tsx`, `${relative}.ts`, @@ -1130,6 +1190,34 @@ describe('server components take only components from client modules', () => { } } + /* + * A local import that resolves to nothing is an edge the walk drops, and everything behind it with + * it — silently, because a missing edge looks exactly like a module that imports less. Review + * found one way in: `~` maps to the app root, and a root-level file such as `proxy.ts` is outside + * every walked directory. `prebundled/`, the deliberately excluded `scripts/`, and any directory + * added later are the same hole. So rather than walking one more place, this fails when a server + * module reaches *any* place the walk does not cover, and says which. + */ + it('drops no local import from the server graph', () => { + const dropped: string[] = []; + + for (const file of serverGraph) { + for (const { specifier } of references(file)) { + const target = localPath(specifier, file); + if (target === null || resolveImport(specifier, file)) continue; + // A stylesheet or an image resolves to a real file this walk has no reason to read. + if (path.extname(target) && !SOURCE_EXTENSION.test(target) && existsSync(path.join(ROOT, target))) continue; + dropped.push(`${file} -> ${specifier}`); + } + } + + expect( + dropped, + 'These imports point at source this walk does not load, so everything behind them is unchecked. ' + + 'Add the directory to SOURCE_DIRS (or the file, if it is at the app root).' + ).toEqual([]); + }); + it('reaches a server graph worth checking', () => { expect(serverGraph.size).toBeGreaterThan(100); }); @@ -1331,6 +1419,23 @@ function verdictFor(names: string[], kind: ExportKind): { label: string; capital * a route nothing imports, and offences found through it would be against code the server never * renders. */ +describe('localPath', () => { + it.each([ + ['~/core/io/rest', 'app/layout.tsx', 'core/io/rest'], + ['./tabs', 'app/space/[id]/layout.tsx', 'app/space/[id]/tabs'], + // The root-level case review raised: outside every walked directory, and still a local path. + ['../proxy', 'app/layout.tsx', 'proxy'], + ['~/proxy', 'app/layout.tsx', 'proxy'], + ['../styles/styles.css', 'app/layout.tsx', 'styles/styles.css'], + ])('points %s from %s at %s', (specifier, from, expected) => { + expect(localPath(specifier, from)).toBe(expected); + }); + + it.each(['react', 'next/navigation', '@geogenesis/auth'])('leaves the package %s alone', specifier => { + expect(localPath(specifier, 'app/layout.tsx')).toBeNull(); + }); +}); + describe('SERVER_ENTRY', () => { it.each([ 'app/layout.tsx', @@ -1554,6 +1659,29 @@ describe('callReferences', () => { expect(refs(source)).toEqual([]); }); + it.each([ + ['a parameter', "function load(require) { return require('./x'); }"], + ['a destructured parameter', "const load = ({ require }) => require('./x');"], + ['a local in the enclosing function', "function load() { const require = pick(); return require('./x'); }"], + ['a function declared in the enclosing block', "{ function require(p) { return p; } require('./x'); }"], + ["a function expression's own name", "const load = function require() { return require('./x'); };"], + ['a catch variable', "try { go(); } catch (require) { require('./x'); }"], + ['a for-of variable', "for (const require of loaders) require('./x');"], + ['an import', "import { require } from './loader';\nrequire('./x');"], + ])('does not take a require declared as %s for the CommonJS one', (_label, source) => { + // Every `require` was read by its spelling, so each of these invented an edge to `./x` — and an + // offence, had `./x` said `use client`. + expect(refs(source).map(({ specifier }) => specifier)).not.toContain('./x'); + }); + + it.each([ + ['beside a function whose parameter is called require', "function other(require) {}\nrequire('./x');"], + ['after a block that declared its own', "{ const require = pick(); }\nrequire('./x');"], + ])('still follows the ambient require %s', (_label, source) => { + // A declaration shadows only inside its own scope. Looking too widely would hide real edges. + expect(refs(source)).toContainEqual({ specifier: './x', local: '' }); + }); + it('reads both the export a require names and the name it is bound to', () => { // Losing the local name is how the both-names gate came to reject a component under a capital. expect(refs("const Widget = require('./x').widget;")).toEqual([ From 561fc6fca35b964a385540b4f16657f22844169e Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Fri, 25 Sep 2026 12:20:41 -0700 Subject: [PATCH 23/23] test: retire the getChecked entry now master no longer reads it #2541 rewrote entity-response.ts on master and it no longer imports getChecked from the checkbox, so that piece of debt is paid. The staleness check caught it on the merge commit, which is what it is for: an entry left behind would go on licensing the same import if someone reintroduced it. --- .../entity-page/client-values-in-server-boundaries.test.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index 80fe24aa6a..467ae62bbf 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -500,8 +500,6 @@ function staticReferences(sourceFile: ts.SourceFile): Reference[] { * across two features and wants someone who can look at the bounties board while doing it. * - `read-block-media-dimensions` would return a client reference in place of its empty-dimensions * object. Nothing calls it outside its own test today, so it is a landmine rather than a fault. - * - `entity-response` calls `getChecked` while deriving a response kind. A server caller would - * throw rather than render something wrong, which is the better failure of the two. * - `bounties/config` is inert: `useFeatureFlag` is only ever called from `useBountiesEnabled`, * which is a client hook. The module is in the server graph for `bountiesEnabledForNetwork`. * Untangling it moves a hook out of `config.ts` and repoints nine files, for no behaviour change. @@ -510,7 +508,6 @@ const KNOWN = new Set([ 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_CARD_HEIGHT_PX (from partials/bounties/board-bounty-card.tsx)', 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_GRID_CLASS (from partials/bounties/board-bounty-card.tsx)', 'core/blocks/data/read-block-media-dimensions.ts -> NO_BLOCK_MEDIA_DIMENSIONS (from core/hooks/use-block-media-dimensions.ts)', - 'core/responses/entity-response.ts -> getChecked (from design-system/checkbox.tsx)', 'core/bounties/config.ts -> useFeatureFlag (from core/state/feature-flags.ts)', ]);