Fix the published lab types and freeze the form-factor constants - #17
Merged
Conversation
…makes
Three gaps in the shipped declarations, each confirmed against a consumer project compiled
under node16 resolution.
LabPlanOptions declared only the plan-level controls, so the published type rejected every
per-run option runLabPlan forwards — TS2353 on skipAudits, and the same for categories,
blockedUrlPatterns, stripJsonProps, clean and silent. Those are exactly the options
bin/web-perf.js passes. The controls are now LabPlanControls, the forwarded options are
LabWriteOptions (LabAuditOptions plus clean and the internally-set runNumber), and
LabPlanOptions is their intersection.
LabReport omitted environment, runtimeError and configSettings. stripJsonProps drops only
i18n and timing, so all three survive by default; buildRunSummary reads
environment.benchmarkIndex, runLabPlan reads runtimeError, cleanLabReport reads
configSettings.formFactor, and the README tells consumers the report carries them. Reading
any of the three was TS2339.
LabPlanHooks.onSummary typed its argument as bare `object`, so the CLI's own
`summary.stability.warnings` would not compile for a consumer. It is now
import('./variance').RunSummary, which this file already referenced in writeRunSummary.
CRUX_FORM_FACTORS and DEFAULT_CRUX_FORM_FACTORS are exported from both crux and crux-history as the same array instance, and DEFAULT_CRUX_FORM_FACTORS is also the default parameter value inside the client. A consumer calling push on it would have added a form factor to every later runCrux and runCruxHistory call in the process — silently increasing requests per URL against a 25,000/day quota. lib/prompts.js already spread the array defensively, so the hazard was understood but never closed. Both are now frozen, with tests asserting the freeze, the shared identity that made it matter, that a push throws, and that spreading to extend still works. Type-level change worth naming: DEFAULT_CRUX_FORM_FACTORS is now `readonly string[]` rather than `string[]`, so a consumer assigning it to a mutable string[] will need to spread. That is the type telling the truth about what the value has always been.
npm test exercises the implementation; nothing checked that the shipped .d.ts files describe it. All four defects in this issue reached a release that way, and the crux/crux-history option types regressed the same way in #15. type-tests/consumer.ts imports the package by name, so it resolves through package.json "exports" exactly as a consumer's build would, and exercises the surface the README documents: runLabPlan with every CLI option and all four hooks, the LabReport fields that survive stripJsonProps, buildRunSummary, both crux record shapes, and the readonly defaults. It is type-checked only — never executed, never published (top-level, so outside the "files" list). Run with `npm run check-types`. Confirmed load-bearing by reverting each fix in turn and re-running the guard: LabPlanOptions loses the run options -> caught onSummary reverts to bare object -> caught LabReport loses environment -> caught
The checklist ended at generate-types, which emits the declarations but never checks them. check-types is now the fourth step, and runs last because it compiles against what generate-types just wrote. Extends the JSDoc rule with the reason this issue existed: npm test exercises the implementation, not the .d.ts, so a published type can be wrong while all 565 tests pass — LabPlanOptions rejected every option the CLI itself passes. Adding an option, a return field or a hook argument now means extending type-tests/consumer.ts, or the guard misses it.
…ript
DEFAULT_CRUX_FORM_FACTORS was declared `string[]`, and `string` is not assignable to
CruxFormFactor, so passing the constant to the `formFactors` option it is the default for has
never compiled — `runCrux(url, key, { formFactors: DEFAULT_CRUX_FORM_FACTORS })` failed with
TS2322 on main and still failed after the freeze, only with TS4104 instead.
Worse, the workaround the freeze commit prescribed in both the code comment and the commit
message — "spread first, as lib/prompts.js does" — did not compile either, because a spread of
a `string[]` is still a `string[]`. The export was unusable from TypeScript in every form.
The constant now carries its element type (`readonly CruxFormFactor[]`), and
CruxRunOptions.formFactors and CruxBatchOptions.formFactors accept a readonly array, so both
the constant and a spread of it type-check. The comment no longer promises a fix that does not
work.
Also declares CruxMetric, which the README's exported-types table listed but which existed
nowhere; it aliases chromeuxreport_v1.Schema$Metric.
Folding LabWriteOptions into LabPlanOptions also published two options runLabPlan owns, and
in both cases a caller-supplied value survives rather than being overridden:
- runNumber is only replaced when repeats > 1, so `runLabPlan(urls, runs, { runNumber: 5 })`
stamps every report in a single-run plan with the same `-run05` suffix; they avoid
overwriting each other only because buildFilename appends `_NN` on collision. The typedef
comment already said runNumber was not for callers — the type contradicted it.
- port is only replaced when the plan launched its own Chrome, so passing it with
reuseBrowser false attaches every run to the caller's single browser. That is the
position-dependent scoring reuseBrowser warns about, reached with no warning at all.
Both are now excluded via Omit. Also drops the trailing `-` from three @Property tags, which
was emitting doc comments whose first line was a bare dash into the published declarations.
README line 532 tells consumers to write
`import type { CruxReport, PsiReport, LabReport } from '@hugoer/web-perf-cli'`. That failed
with TS2305: lib/index.js is a value-only façade of lazy getters, so types/lib/index.d.ts
declared 23 functions and not one type — while package.json's top-level "types" field points
at exactly that file.
The root now re-declares every row of the README's exported-types table, so the documented
import compiles.
Found by extending the type guard to cover the package root, which it had never imported.
… catch The guard shipped with holes in exactly the places it was built to protect. It never imported the package root, so types/lib/index.d.ts — the file package.json's "types" field names — was the one declaration set it did not check. Covering it immediately found the README's documented root type import failing. It also skipped psi, sitemap, links, utils and profiles entirely. Its freeze assertion was vacuous: `const extended: string[] = [...DEFAULT, 'tablet']` compiles whether the source is readonly or not, because spreading a readonly array yields a mutable one. It is now a @ts-expect-error on a push, plus calls passing the constant to both CruxRunOptions and CruxBatchOptions — a spread compiles against either shape, so it covered neither typedef. runLabToDisk's parameter narrowed from `object` to LabWriteOptions in the previous commit with no call added, breaking the rule that same commit put in CLAUDE.md. A valid call cannot catch that widening back either, since every object literal is assignable to `object`; rejecting an unknown property can. skipLibCheck is off, so errors inside the shipped .d.ts files are no longer suppressed — the opposite of what a declaration guard should do. Every assertion was mutation-tested. Seven reverts, seven failures: un-freeze DEFAULT_CRUX_FORM_FACTORS -> caught narrow CruxRunOptions.formFactors -> caught runNumber/port back into LabPlanOptions -> caught drop CruxReport from the root -> caught runLabToDisk options back to `object` -> caught LabPlanOptions loses the run options -> caught onSummary reverts to bare object -> caught Two of those (formFactors, runLabToDisk) passed on the first attempt and were strengthened.
check-types existed but nothing ran it: lint.yml runs lint, test.yml runs tests, and its only enforcement was a line in CLAUDE.md. A contributor running `npm run lint && npm test` saw two green workflows and could merge broken declarations — the exact path every defect in #16 took. Nothing verified types/ was regenerated either, so check-types could compile against a stale committed declaration set and report green while the published types drifted from the implementation. The new workflow does both: regenerate, fail on any diff under types/ with a message naming the fix, then run check-types. Verified locally that the drift gate fires on a JSDoc change made without regenerating.
Splitting LabPlanOptions introduced LabPlanControls and LabWriteOptions, and changed what LabPlanOptions itself means — from controls-only to controls plus per-run options, minus the two runLabPlan owns. The Scripts table was updated but the exported-types table was not, which is the first of the three README staleness points CLAUDE.md names. Adds all four lab option types, including what LabPlanOptions now excludes and why.
TypeScript documents that the "types" condition must come first, because conditions are matched in order. It was listed last in all ten subpaths, so tsc matched "require", found no .d.ts beside the .js, and only then fell back to "types". Confirmed with --traceResolution: every subpath now reports `Matched 'exports' condition 'types'` directly, with no failed require attempt. It resolved correctly by accident. The day anything emits declarations next to lib/*.js — or a consumer toolchain does not implement that fallback — every subpath would silently resolve to untyped JS while check-types still passed, because the guard exercises one resolution mode.
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16.
Four type defects in the published declarations, plus a guard so this class of bug stops reaching releases.
The defects
All four were confirmed against a consumer project compiled under
node16resolution — the exacttscerrors are in the issue.LabPlanOptionsrejected every option the CLI passes. It declared only the plan-level controls, butrunLabPlanforwards everything else into each run.runLabPlan(urls, runs, { skipAudits })— a callbin/web-perf.jsmakes on every invocation — did not compile for a consumer. Split intoLabPlanControlsandLabWriteOptions, withLabPlanOptionstheir intersection.LabReportomitted three fields that survivestripJsonProps. It strips onlyi18nandtiming, soenvironment,runtimeErrorandconfigSettingsare all present by default — and this repo reads all three (buildRunSummary→environment.benchmarkIndex,runLabPlan→runtimeError,cleanLabReport→configSettings.formFactor). The README advertises them too.onSummarytyped its argument as bareobject. The CLI's ownsummary.stability.warningswould not compile for a consumer. NowRunSummary, whichlib/lab.jsalready referenced elsewhere.The form-factor constants were mutable and shared.
DEFAULT_CRUX_FORM_FACTORSis one instance behind both subpaths and the default parameter value inside the client — a consumer'spushwould have added a form factor to every later call in the process, against a metered quota. Both frozen.The guard
npm testexercises the implementation; nothing checked that the.d.tsfiles describe it. All four defects shipped that way, and the crux option types regressed identically in #15.type-tests/consumer.tsimports the package by name, so it resolves throughpackage.json"exports" exactly as a consumer's build would — no symlink, no fixture project. It exercises the surface the README documents:runLabPlanwith every CLI option and all four hooks, theLabReportfields that survive stripping,buildRunSummary, both crux record shapes, and the readonly defaults. Type-checked only, never executed, and top-level so it stays outside the publishedfileslist.Wired as
npm run check-typesand added as the fourth step of the checklist in CLAUDE.md and the README, aftergenerate-typessince it compiles what that emits.Verification
Both new test layers were confirmed load-bearing by breaking the thing they protect, not just by passing.
Reverting each fix in turn, with the guard re-run:
npm run lint— 0 problems.npm test— 565 passing, up from 561. Four new tests cover the freeze, the shared identity that made it matter, that a push throws, and that spreading still extends.npm run generate-types— committed, no drift on a re-run.npm run check-types— passes.lab-audit,crux-audit,crux-history-auditandlab-audit-varianceexamples re-run green against the working tree.