diff --git a/.changeset/8454-runner-stylesheet-source-set.md b/.changeset/8454-runner-stylesheet-source-set.md new file mode 100644 index 0000000000..beee2382e7 --- /dev/null +++ b/.changeset/8454-runner-stylesheet-source-set.md @@ -0,0 +1,38 @@ +--- +'@object-ui/runner': patch +--- + +Fix every `@source` path in the runner's Tailwind entry, and keep test files out +of its published stylesheet (objectui#8454). + +`packages/runner/src/index.css` is a published input: this package is +`private: false` with `files: ["dist"]`, so what it compiles to is bytes a +consumer downloads and serves. All five of its `@source` lines were wrong by one +path segment. Tailwind resolves a relative `@source` against the directory of the +entry CSS — `packages/runner/src/` — so `./src/**` meant +`packages/runner/src/src` and `../../packages//src` meant +`packages/packages//src`. A glob whose base directory does not exist scans +nothing and raises no error, so the file read as if it declared its inputs while +declaring none: deleting all five lines produced a byte-identical artifact. + +What kept the sheet non-empty was Tailwind's automatic source detection, whose +base defaults to the process CWD — `packages/runner`, where `pnpm build` runs. +That covers this package's own tree and nothing else, so every utility belonging +to `@object-ui/components`, `@object-ui/react`, `@object-ui/plugin-kanban` and +`@object-ui/plugin-charts` was missing from the shipped app. Measured from the +package directory, repairing the four sibling paths takes the compiled sheet from +228 to 1456 selectors (21 kB to 136 kB): `bg-popover`, `bg-accent`, +`bg-destructive`, `animate-out` and 1224 more had no source anywhere. + +The same automatic root also swept this package's own test files into the +published bytes. Two `@source not` lines — the spelling +`packages/plugin-kanban/src/index.css` already uses, anchored one level up +because this entry scans four sibling packages — remove nine test-sourced +classes, several of them ordinary English words lifted out of prose comments +(`paused`, `invert`, `flex-nowrap`). + +`patch`: this package exposes no importable surface at all (no `main`, `module`, +`types` or `exports` — it publishes a built application under `dist`), so nothing +a consumer imports changes shape. The nine removed classes are unreachable from +the app's own markup by construction, which is why the scan never had a shipped +source for them. diff --git a/packages/runner/src/__tests__/published-stylesheet-sources.test.ts b/packages/runner/src/__tests__/published-stylesheet-sources.test.ts new file mode 100644 index 0000000000..f6949bd00d --- /dev/null +++ b/packages/runner/src/__tests__/published-stylesheet-sources.test.ts @@ -0,0 +1,180 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * Pins the SOURCE SET of `@object-ui/runner`'s stylesheet (objectui#8454). + * + * This package is `private: false` with `files: ["dist"]`, so `src/index.css` + * compiles into bytes a consumer installs. Two things about it went wrong at + * once, and neither one produced any error at build time: + * + * 1. every `@source` path named a directory that does not exist. Tailwind + * resolves a relative `@source` against `dirname()` — here + * `packages/runner/src/` — so `./src/**` meant `packages/runner/src/src` + * and `../../packages//src` meant `packages/packages//src`. A + * glob whose base is missing scans nothing and is not an error: deleting + * all five lines was measured byte-identical to keeping them. + * 2. nothing excluded test files. What kept the sheet non-empty was Tailwind's + * automatic source detection, which roots at the PROCESS CWD (`base` + * defaults to `process.cwd()`), i.e. `packages/runner` when `pnpm build` + * runs — so runner's own tests were scanned and shipped, down to ordinary + * English words picked out of prose comments. + * + * ## Why a naive assertion passes while broken + * + * "The published sheet contains no test-sourced class" is satisfied perfectly + * by a sheet compiled from NOTHING — which is exactly the state defect (1) put + * this package in for the four sibling trees. So the negative assertion below + * is paired with a positive one that a sheet compiled from nothing fails: a + * themed utility that ONLY `@object-ui/components`' shipped source can supply. + * And the negative is not a bare absence either — it is a DIFFERENCE against a + * second compile of the same entry with the `@source not` lines stripped, so an + * exclusion that silently stopped matching anything fails here rather than + * passing quietly. + * + * ## Why it compiles instead of reading `dist/` + * + * CI runs the suite on an unbuilt worktree, so `dist/` is legitimately absent + * and a test that read the artifact would pass vacuously — the same reasoning + * `scripts/__tests__/plugin-published-stylesheet.test.ts` records. It runs the + * real `@tailwindcss/postcss` over the real entry instead, with `base` pinned + * to the package root so the automatic-detection root is the one `pnpm build` + * uses no matter which directory the suite was launched from. + */ +import { describe, expect, it } from 'vitest'; +import { existsSync, readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import postcss from 'postcss'; +import tailwindPostcss from '@tailwindcss/postcss'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +/** `packages/runner` — what `process.cwd()` is when this package builds. */ +const PACKAGE_ROOT = resolve(HERE, '../..'); +const ENTRY = resolve(PACKAGE_ROOT, 'src/index.css'); +const ENTRY_DIR = dirname(ENTRY); + +const entryCss = readFileSync(ENTRY, 'utf8'); + +/** + * Class names in a compiled selector, undoing Tailwind's CSS escapes. + * Mirrors `classesIn` in `scripts/build-plugin-stylesheet.mjs`; not imported + * from there because the root tsconfig sets `allowJs: false`, so a `.ts` test + * under `packages/` cannot pull in a plain `.mjs` helper. + */ +function classesIn(selector: string): string[] { + const found: string[] = []; + const re = /\.((?:\\.|[^\s.,>+~()[\]:#*'"\\])+)/g; + let m: RegExpExecArray | null; + while ((m = re.exec(selector))) { + found.push( + m[1] + .replace(/\\([0-9a-fA-F]{1,6})\s?/g, (_, hex: string) => + String.fromCodePoint(parseInt(hex, 16)), + ) + .replace(/\\(.)/g, '$1'), + ); + } + return found; +} + +/** + * Compile the entry the way this package's build does. `base` is the + * automatic-detection root and defaults to `process.cwd()` — pinning it to the + * package root is what makes this reading independent of where vitest started + * (from the repo root the same bytes scan the whole monorepo instead). + */ +async function compileClasses(css: string): Promise> { + const result = await postcss([tailwindPostcss({ base: PACKAGE_ROOT })]).process(css, { + from: ENTRY, + }); + const classes = new Set(); + postcss.parse(result.css, { from: ENTRY }).walkRules((rule) => { + for (const selector of rule.selectors) for (const cls of classesIn(selector)) classes.add(cls); + }); + return classes; +} + +/** `@source` / `@source not` directives, in file order, params unquoted. */ +function sourceDirectives(css: string): { negated: boolean; pattern: string }[] { + const out: { negated: boolean; pattern: string }[] = []; + for (const line of css.split('\n')) { + const m = /^@source\s+(not\s+)?['"](.+)['"]\s*;/.exec(line.trim()); + if (m) out.push({ negated: Boolean(m[1]), pattern: m[2] }); + } + return out; +} + +/** The literal directory prefix of a glob — everything before the first `*`. */ +function globBase(pattern: string): string { + const star = pattern.indexOf('*'); + const literal = star === -1 ? pattern : pattern.slice(0, star); + return resolve(ENTRY_DIR, literal.replace(/\/[^/]*$/, '')); +} + +// Cost paid at import time on purpose: module evaluation is bound by no test or +// hook timeout, and each compile is ~0.6s. AGENTS.md's testing-discipline +// section is explicit that a `beforeAll` would be the WORSE place for it: +// `hookTimeout` (10s) is narrower than `testTimeout` (15s). +const shipped = await compileClasses(entryCss); +const withTestSources = await compileClasses( + entryCss + .split('\n') + .filter((line) => !line.trimStart().startsWith('@source not ')) + .join('\n'), +); + +describe('@object-ui/runner published stylesheet — source set', () => { + it('resolves every @source path to a directory that exists', () => { + const directives = sourceDirectives(entryCss); + // Non-vacuity: the check above is trivially satisfied by a file with no + // `@source` at all, which is the shape this card had to rule out. + expect(directives.filter((d) => !d.negated).length).toBeGreaterThanOrEqual(5); + + const missing = directives + .map((d) => ({ ...d, base: globBase(d.pattern) })) + .filter((d) => !existsSync(d.base)); + expect(missing).toEqual([]); + }); + + it('the historical spellings really did point at nothing (control)', () => { + // Without this, the assertion above could be green because every path + // resolves to the package dir by accident rather than by being correct. + expect(existsSync(resolve(ENTRY_DIR, 'src'))).toBe(false); + expect(existsSync(resolve(ENTRY_DIR, '../../packages/components/src'))).toBe(false); + }); + + it('compiles utilities that only a dependency can supply', () => { + // `bg-popover` is used by ten shipped files under `packages/components/src` + // (popover, dropdown-menu, tooltip, select, …) and by nothing in this + // package's own `src`. It can therefore only come from the + // `../../components/src/**` line, and it is absent the moment that line + // stops resolving — which is what shipped before objectui#8454. + expect(shipped).toContain('bg-popover'); + expect(readFileSync(resolve(PACKAGE_ROOT, 'src/LayoutRenderer.tsx'), 'utf8')).not.toContain( + 'bg-popover', + ); + // Lit control for the two assertions above: a class this package's OWN + // source supplies, so a compile that produced nothing cannot pass either. + expect(shipped).toContain('flex-col'); + }); + + it('keeps test-sourced classes out of the published sheet', () => { + // `paused` reaches the candidate list only as an English word inside a + // prose comment in `packages/components/src/__tests__/`. It is a real + // Tailwind utility (`animation-play-state: paused`), so without the + // exclusions it compiles into bytes every consumer downloads. + expect(withTestSources).toContain('paused'); + expect(shipped).not.toContain('paused'); + + // And the exclusions must still be subtracting something at all: an + // `@source not` that quietly stopped matching would otherwise leave every + // assertion in this file green. + const removed = [...withTestSources].filter((cls) => !shipped.has(cls)); + expect(removed.length).toBeGreaterThan(0); + expect([...shipped].filter((cls) => !withTestSources.has(cls))).toEqual([]); + }); +}); diff --git a/packages/runner/src/index.css b/packages/runner/src/index.css index 99b6f59533..dd5f1a187a 100644 --- a/packages/runner/src/index.css +++ b/packages/runner/src/index.css @@ -6,12 +6,57 @@ shared styles entry. Keep in sync with packages/app-shell/src/styles.css. */ @custom-variant dark (&:where(.dark, .dark *)); -/* Scan sources for Tailwind classes */ -@source './src/**/*.{ts,tsx}'; -@source '../../packages/components/src/**/*.{ts,tsx}'; -@source '../../packages/react/src/**/*.{ts,tsx}'; -@source '../../packages/plugin-kanban/src/**/*.{ts,tsx}'; -@source '../../packages/plugin-charts/src/**/*.{ts,tsx}'; +/* + * Scan sources for Tailwind classes. + * + * ⚠️ Every path below is resolved relative to THIS FILE's directory + * (`packages/runner/src/`) — not the package root, not the CWD. + * `@tailwindcss/postcss` compiles with `base = dirname()`, and the + * `@source` params are rewritten against that same base before the scanner ever + * sees them. All five lines used to get it wrong by exactly one `src` segment + * (`./src/**` -> `packages/runner/src/src`, `../../packages//src` -> + * `packages/packages//src`), so every one of them named a directory that + * does not exist and contributed nothing: measured on objectui#8454, deleting + * all five produced a byte-identical artifact. + * + * What kept this sheet non-empty was Tailwind's automatic source detection, + * which roots at the PROCESS CWD — `packages/runner`, where `pnpm build` runs. + * That covers runner's own tree and nothing else, so the utilities its + * dependencies need were simply missing from the published bundle: fixing the + * four sibling paths takes it from 228 to 1456 compiled selectors (21 kB -> + * 136 kB). The automatic root is still on (there is no `source(none)` here, and + * that parity question is objectui#8455), which is why the first line below is + * a no-op today and still belongs here: it is the declared intent, and it is + * what keeps runner's own source scanned the day the root moves. + */ +@source './**/*.{ts,tsx}'; +@source '../../components/src/**/*.{ts,tsx}'; +@source '../../react/src/**/*.{ts,tsx}'; +@source '../../plugin-kanban/src/**/*.{ts,tsx}'; +@source '../../plugin-charts/src/**/*.{ts,tsx}'; + +/* + * Only shipped source. A `*.test.tsx` never reaches a consumer, and neither + * does a helper that lives beside one, so a utility used solely by tests must + * not become a published byte. Same two-line spelling as + * `packages/plugin-kanban/src/index.css`, anchored one level up because this + * entry scans four sibling packages instead of only its own tree — `../../` + * bases these on `packages/`, which contains every tree named above AND the + * automatic-detection root, so runner's own tests are covered by the same two + * lines (measured: negations do apply to files the automatic root found). + * + * Both lines earn their place. The file line removes nine test-sourced classes + * (`flex-grow`, `flex-nowrap`, `h-[125px]`, `h-[400px]`, `invert`, `paused`, + * `slide-in-from-bottom`, `text-green-500`, `w-[250px]` — several of them + * ordinary English words picked out of prose comments). The directory line + * removes nothing extra today, but it is the only thing covering the + * non-`*.test.*` sources under `packages/components/src/__tests__/` + * (`test-utils.tsx`, `page-header-action-ids.dist.spec.tsx`); ablated with a + * marker class injected into the first of those, the file line alone ships it + * and the pair does not. + */ +@source not '../../**/*.test.{ts,tsx}'; +@source not '../../**/__tests__/**'; /* Tailwind plugin for animations */ @plugin 'tailwindcss-animate'; diff --git a/packages/runner/tsconfig.test.json b/packages/runner/tsconfig.test.json index c3b5e57381..dccf88a86d 100644 --- a/packages/runner/tsconfig.test.json +++ b/packages/runner/tsconfig.test.json @@ -17,7 +17,14 @@ // pulls `lib/MetadataLoader.ts` into this program, and that file calls // `import.meta.glob` — declared by `vite/client`, which the root config // this extends does not carry (it is not a Vite app). - "types": ["vite/client"] + // + // `node` joins it for `src/__tests__/published-stylesheet-sources.test.ts`, + // which reads and compiles this package's Tailwind entry through + // `node:fs` / `node:path` / `node:url`. Naming `types` at all switches off + // automatic `@types/*` inclusion, so the entry is required rather than + // implied; `@types/node` resolves from the workspace root, which is how + // `packages/components/tsconfig.test.json` gets it too. + "types": ["node", "vite/client"] }, "include": ["src/**/*.test.ts", "src/**/*.test.tsx"] }