Freeze the remaining exported constants that double as default arguments - #19
Merged
Merged
Conversation
PSI_STRATEGIES and DEFAULT_PSI_STRATEGIES are exported from web-perf-cli/psi and are also the default `strategies` for runPsi, runPsiBatch and runPsiAuditBatch. A consumer calling push on DEFAULT_PSI_STRATEGIES would have added a strategy to every later call in the process — an extra API request per URL against the 25,000/day quota. DEFAULT_PSI_CATEGORIES is internal but is a default argument in the same way, so it is frozen alongside them. Each carries its element type (`readonly PsiStrategy[]`), not bare `string[]`, so the exported constant can still be passed to the option it is the default for. That was the mistake in the CrUX freeze: declared `string[]`, the constant could never be passed back into its own option, and freezing only changed the error code. The strategies options accept a readonly array for the same reason. The two batch option objects are promoted to named typedefs (PsiBatchOptions, PsiWriteBatchOptions) because the inline forms went past the line limit once widened.
Both are exported from web-perf-cli/lab and the package root. DEFAULT_SKIP_AUDITS is the fallback inside buildLighthouseConfig, so mutating it silently skips an extra audit in every later run. CHROME_FLAGS is the sharper one: it is handed to every Chrome launch in lab.js and links.js, so appending a flag changes how every subsequent audit launches the browser — invalidating scores rather than merely costing quota. chrome-launcher types chromeFlags as a mutable Array<string>, so the three call sites now spread. Consumers passing CHROME_FLAGS to chromeLauncher.launch() must do the same; the constant's comment says so, and type-tests asserts the spread keeps compiling. Verified with real browser launches: the links command and the lab-audit and lab-save-runs examples all still start Chrome and write output.
Exported from web-perf-cli/profiles and used to validate --category input, so mutating it changes which categories the CLI accepts for the rest of the process.
Each constant is asserted three ways, because freezing alone is not the property that matters: pass it into the option it is the default for, spread it to extend, and fail to mutate it. The middle assertion is the one the CrUX pair failed — declared `string[]`, the constant could not be passed back into `formFactors`, so it was frozen and unusable at the same time. Mutation-tested, six reverts and six failures: un-freeze PSI_STRATEGIES -> caught un-freeze DEFAULT_PSI_STRATEGIES -> caught narrow PsiBatchOptions.strategies -> caught un-freeze DEFAULT_SKIP_AUDITS -> caught un-freeze LAB_CATEGORIES -> caught un-freeze CHROME_FLAGS -> caught
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
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 #18.
Freezes the five exported constants that double as default arguments, applying the same pattern — and the same lesson — as the CrUX pair in #17.
What was exposed
Each of these is exported from a public subpath and used as a default parameter value, so a consumer mutating one changed behaviour for every later call in the process:
DEFAULT_PSI_STRATEGIESpsirunPsi,runPsiBatch,runPsiAuditBatchPSI_STRATEGIESpsiDEFAULT_SKIP_AUDITSlab, rootbuildLighthouseConfigCHROME_FLAGSlab, rootlab.jsandlinks.jsLAB_CATEGORIESprofiles--categoryvalidationDEFAULT_PSI_CATEGORIESis internal rather than exported, but is a default argument in the same way, so it is frozen alongside them.CHROME_FLAGSis the sharpest of the five. The others cost quota — an extra request per URL against 25,000/day. Appending a flag toCHROME_FLAGSchanges how every subsequent audit launches the browser, which silently invalidates scores.Freezing alone would have repeated #17's mistake
DEFAULT_CRUX_FORM_FACTORSwas frozen in #17 but declaredstring[], andstringis not assignable toCruxFormFactor— so the constant could never be passed back into the option it was the default for. It was frozen and unusable at the same time, and the freeze only changed the error code from TS2322 to TS4104.So each constant here gets both halves: an element type (
readonly PsiStrategy[], notreadonly string[]), and the options that receive it widened to accept a readonly array.The
chrome-launcherquestion the issue flaggedAnswered:
chrome-launchertypeschromeFlagsas a mutableArray<string>, so a frozenCHROME_FLAGScannot be passed to it directly. The three call sites inlab.jsandlinks.jsnow spread, the constant's comment says consumers must do the same, andtype-testsasserts that spread keeps compiling.That is a real ergonomic cost on the most likely use of the export, and it is the one judgement call in this PR. I took it because the alternative is an array that every audit launch reads and any consumer can quietly edit.
Verification
Every assertion mutation-tested — six reverts, six failures:
npm run lint— 0 problems.npm test— 565 passing, unchanged.npm run generate-types— committed, no drift.npm run check-types— passes.chromeFlagsspread is a runtime change: thelinkscommand wrote output, andlab-audit,lab-save-runs,psi-auditandpsi-saveexamples all pass.Deliberately out of scope
PROFILES,NETWORK_PRESETSandDEVICE_PRESETSinlib/profiles.jsare exported objects read byresolveProfileSettings, soPROFILES.low.network = 'wifi'would change every later audit — the same hazard class. They need a deep freeze, sinceObject.freezewould leave the nested preset objects writable, and that has different type implications. Filed separately rather than smuggled in here.