fix(profiles): deep-freeze the exported preset objects - #21
Merged
Conversation
PROFILES, NETWORK_PRESETS and DEVICE_PRESETS are exported from
web-perf-cli/profiles and read by resolveProfileSettings on every audit, so a
consumer mutating one changes what every later run measures:
PROFILES.low.network = 'wifi';
Every subsequent `lab --profile=low` then runs on WiFi instead of Regular 3G
while the report still says the profile was "low". Nothing signals it, which
makes it worse than a failed run. NETWORK_PRESETS is the sharpest case —
buildThrottling reads rttMs, throughputKbps, uploadKbps and
cpuSlowdownMultiplier straight out of it, the values the INVARIANT note is
about.
Object.freeze is the wrong depth here: it stops PROFILES.low = {...} but leaves
PROFILES.low.network writable, and the nested write is the damaging one. A local
deepFreeze recurses into each preset. It stays unexported — lib/utils.js is a
published subpath, and a helper serving one module does not belong on the
package's semver surface.
The emitted declarations are unchanged by this commit; publishing the objects as
readonly is a separate change.
The previous commit froze the three preset maps at runtime; a consumer's TypeScript build still saw them as mutable object literals, so the assignment that now throws compiled cleanly. Each map is annotated as `Readonly<Record<keys, Readonly<Preset>>>` over a named typedef — NetworkPreset, DevicePreset, LabProfile. The properties are all primitives, so shallow Readonly on the entry is deep enough here. Naming the entry shapes also shrinks the declarations: they described a preset once per key before, six times over for NETWORK_PRESETS and DEVICE_PRESETS. Two side effects of the annotations are improvements rather than accidents. PROFILES had been emitted as a namespace of `let` bindings with generated aliases (network_1, network_2 …) because it was a bare object literal; it is now a single const. DevicePreset.formFactor narrows from string to 'mobile' | 'desktop', which is what the two values have always been. The three types are re-declared in lib/index.js and listed in the README's "Key exported types" table, alongside a note on why the maps are frozen and how to build a variant by spreading.
Each of the three maps is asserted five ways at runtime and five ways in type-tests: read a nested value, spread an entry into a variant, fail to write a nested property, fail to replace a whole entry, and — at runtime — confirm resolveProfileSettings still resolves to the same numbers. lib/profiles.test.js is an ES module, so it runs in strict mode and a frozen write throws rather than failing silently. A sloppy-mode CommonJS consumer gets the silent no-op instead; either way the mutation never lands, which is the property under test. Eleven mutations, all caught, none survived: - deepFreeze stops recursing -> 10 runtime tests fail - each map left unfrozen (x3) -> 4-5 runtime tests fail - entry type loses Readonly (x3) -> TS2578 unused @ts-expect-error - map type loses Readonly (x3) -> TS2578 unused @ts-expect-error - DevicePreset.formFactor widened -> TS2322 at the assignment The two Readonly layers are tested separately because they block different writes: dropping the inner one re-opens PROFILES.low.network, dropping the outer one re-opens PROFILES.low. Only the inner failure is silent, which is why it is the one worth a dedicated mutation. type-tests/root.ts gains the three new types, keeping its stated invariant true: every row of the README's "Key exported types" table resolves from the root.
deepFreeze froze the parent AFTER recursing into its values, so a self-referential
object recursed forever:
const a = { child: {} };
a.child.parent = a;
deepFreeze(a); // RangeError: Maximum call stack size exceeded
Not reachable from this module — the three preset maps are literals with no cycles
and no shared references — but the helper is written generically and reads as a
utility, so the next caller would have inherited it.
Freezing the parent first is what makes an Object.isFrozen guard work at all: with
the original order the parent is still unfrozen when the recursion re-enters it, so
the guard never fires and the cycle is not broken. Reordered, the guard both
terminates the cycle and skips subtrees that are already frozen.
Verified against `a.child.parent = a`: the old body raises RangeError, the new one
returns with both levels frozen.
The "rejects replacing a whole entry" case guarded with toBeTypeOf('object'), which
`{}` satisfies — the exact value the write attempts. The assertion could not fail for
the mutation the case is named after.
Capturing the entry first and asserting identity against it does. This changes no
outcome under ESM, where the write throws and toThrow already carries the case; it
matters for the sloppy-mode CommonJS consumer the block's header comment reasons
about, where the write is a silent no-op and the identity check is the only assertion
left standing. The sibling nested-write case already worked this way.
Re-checked against the map-left-unfrozen mutation: still 5 failures for PROFILES.
…pe reference Two consumer-facing documentation gaps around the frozen constants. README described the three preset objects as frozen and said nothing about the seven frozen arrays — CHROME_FLAGS, DEFAULT_SKIP_AUDITS, LAB_CATEGORIES, PSI_STRATEGIES, DEFAULT_PSI_STRATEGIES, CRUX_FORM_FACTORS, DEFAULT_CRUX_FORM_FACTORS. Nothing had documented those since they were frozen, so the note read as if they were still mutable. It now opens on the exported constants as a set, then keeps the deep-freeze rationale in its own paragraph, since only the objects need it. The list was checked against Object.freeze call sites and each module's exports: DEFAULT_PSI_CATEGORIES is frozen but unexported, so it is deliberately absent. NetworkPreset's throughputKbps and uploadKbps pointed at "the INVARIANT above". That note is a // line comment, which tsc drops, so the published declarations carried a reference to nothing — and the declarations are what a consumer reads on hover. Both descriptions are now self-contained and name buildThrottling as the owner of the factors. The buildThrottling reference that survives in the emitted .d.ts names the note's location rather than pointing "above", so it still resolves.
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 #20.
Follow-up to #18, which froze the five exported array constants. The three
exported objects carried the same hazard and were left out because they need a
deep freeze rather than a shallow one.
Behaviour change
PROFILES,NETWORK_PRESETSandDEVICE_PRESETS(fromweb-perf-cli/profiles)are now frozen at every level and published as
readonly. A consumer doingpreviously succeeded, and every later
lab --profile=lowaudit then ran on WiFiinstead of Regular 3G while the report still said the profile was "low" — a
wrong number with nothing signalling it. It now fails to compile, and at runtime
throws under strict mode (ESM, or
'use strict') instead of taking effect.Object.freezealone was not enough: it blocksPROFILES.low = {...}but leavesPROFILES.low.networkwritable, and the nested write is the damaging one. Alocal
deepFreezerecurses into each preset. It stays unexported —lib/utils.jsis a published subpath, and a helper serving one module does not belong on the
package's semver surface.
Nothing in the repo mutated these objects;
lib/prompts.jsonly reads keys.Published types
Each map is annotated
Readonly<Record<keys, Readonly<Preset>>>over a namedtypedef. Three new types ship on
web-perf-cli/profilesand from the root:LabProfile,NetworkPreset,DevicePreset.Two side effects are improvements rather than accidents:
PROFILESwas emitted as a namespace ofletbindings with generated aliases(
network_1,network_2, …) because it was a bare object literal. It is now asingle
const.DevicePreset.formFactornarrows fromstringto'mobile' | 'desktop'.Naming the entry shapes also shrinks
profiles.d.ts: it described a preset onceper key before, six times over for the two preset maps.
Verification
Eleven mutations, all caught, none survived:
deepFreezestops recursingReadonly(x3)@ts-expect-errorReadonly(x3)@ts-expect-errorDevicePreset.formFactorwidenedThe two
Readonlylayers are tested separately because they block differentwrites: dropping the inner one re-opens
PROFILES.low.network, dropping theouter one re-opens
PROFILES.low. Only the inner failure is silent.Beyond the four gates (lint, 581 tests,
generate-typeswith no drift,check-types), a real Lighthouse run through the frozen presets confirms thevalues still reach Lighthouse unchanged:
examples/lab-audit-custom-throttling.jsalso runs clean against the workingtree, exercising the
--network/--devicepath.Not in this PR
type-tests/root.tsstates that every row of the README's "Key exported types"table must resolve from the package root. Four rows predate this branch and do
not:
LabAuditOptions,LabWriteOptions,LabPlanControlsandLabPlanOptionsall fail with TS2305 — the same defect #17 fixed for the reporttypes. Filed separately rather than widened into this PR.