Remove duplicated error handling and collapse the crux/crux-history near-duplicate - #15
Merged
Merged
Conversation
Every action ended with a catch that logged `Error: ${err.message}` and exited 1, and was
also wrapped in withCatch, which does byte-identically the same thing. The inner handler
could never run differently from the outer one, so it was dead weight in seven functions.
Removed from labAction, psiAction, cruxAction, cruxHistoryAction, sitemapAction, linksAction
and wizardMode. withCatch is now the single error policy. labAction's own process.exit(1) for
the failed-run list stays — that is a non-throwing path and withCatch never saw it.
Purely mechanical: `git diff -w` on this commit contains only the seven deleted try/catch
blocks and nothing else. Verified that --runs=0, --runs with --reuse-browser, and a missing
clean input still print the same message and exit 1.
…ommand The wizard's cleanAction and the clean subcommand's inline action were the same seven lines twice, differing only in where the input came from. cleanAction now takes the input as an optional argument and prompts only when it is absent, so the subcommand passes it straight through and the wizard keeps asking. Verified both paths: the subcommand cleans a directory and exits 0, a missing input still exits 1.
…lient crux.js and crux-history.js held four functions each that differed only in the endpoint, the results directory, the label used in error messages, and whether a record carries collectionPeriod or collectionPeriods. Origin normalisation, treating 404 as no-data, retry, rate limiting and file naming were duplicated line for line — which is why the runtimeError and no-data fix in #9 had to be written twice, once per module. createCruxClient in the new lib/crux-client.js now owns that logic once. crux.js and crux-history.js are configuration plus typed wrappers. The wrappers are not ceremony. The factory assembles its record with a computed [periodKey], which erases the concrete return type to `{ [x: string]: any }`. Restating the signature in each module keeps the published types describing a CrUX record: runCruxAudit still returns Promise<CruxReport> and runCruxHistoryAudit still returns Promise<CruxHistoryReport>. The erased shape stays confined to the internal crux-client.d.ts. Not a line-count win — 365 lines becomes 355. The win is that ~250 lines of duplicated logic became ~185 lines that exist once, so the next fix to this path lands in one place. Verified: export surface byte-identical on both modules and through lib/index.js; written JSON key order unchanged (source, scope, formFactor, url, <period>, extractedAt, metrics, key); all 43 crux tests pass untouched. lib/crux-client.js registered in tsconfig.types.json and package.json exports per the project checklist.
…efault The LabReport typedef declared `timing` as a required property, but runLabAudit strips both `timing` and `i18n` unless stripJsonProps is explicitly false. The published .d.ts therefore promised consumers a field that the default path deletes.
buildRunSummary called performanceScore on every successful run three times over — once for `scores`, once for the `scored` filter, and once again for `scoredValues`. The score is now computed once and the two derived arrays filter over it. Both filters use the same predicate in the same order, so `scored` and `scoredValues` remain index-aligned and medianIndex still addresses both.
When continueOnError is false, runLabPlan pushed the failure onto both `group` and `results` and then threw on the next line. The rejection discards both arrays, so neither push could ever be observed. Moved the throw ahead of them. No behaviour change: the hook call and the continueOnError path are untouched, and the onRunError hook was already skipped on an aborting run.
The LighthouseAudit and LighthouseCategory typedefs and runLabAudit's options parameter were single lines of 213, 200+ and 216 characters. Rewritten as @Property blocks, with the options object promoted to a named LabAuditOptions typedef. Clears the last two max-len warnings; combined with the crux refactor, eslint now reports 0 problems across the repo, down from 6 warnings on main.
The Structure block listed 9 of 15 lib modules; clean.js, clean-cmd.js, index.js, logger.js and strip-props.js were already absent and crux-client.js would have made six. Now complete, with a note on why crux.js and crux-history.js keep typed wrappers over the factory. Adds two rules. The first covers examples/ and the README: examples depend on the repo via file:.. so they run against the working tree, but npm test does not touch them — they need network and an API key — so nothing catches a break automatically. A change to an exported name, signature, return shape or CLI behaviour has to be reflected there and the affected examples run by hand. It also records that sitemap, links and clean have no example and no failure path is covered, so a green run is not full coverage. The second says a new lib module belongs in the Structure block as well as in tsconfig.types.json and package.json. All three drift silently.
…rmats Two real gaps found auditing README.md against the code. The sitemap section never documented the same-origin restriction on child sitemaps that shipped in #13. It is user-visible — an index listing a www. variant of its own domain now skips those children with a warning — and it went undocumented precisely because the export surface did not change, so nothing prompted a doc edit. Entity decoding is documented alongside it, since it is the reason a sitemap URL with a query string is now usable in --urls-file. CLAUDE.md's Output block claimed HHMM for psi, crux, crux-history, links and sitemap, and omitted the profile from the lab clean filename. formatDate() has always emitted HHMMSS; README had it right and CLAUDE.md had drifted. Both now agree with the code. The examples/README rule is rewritten around what actually went wrong: it now names the three places README goes stale, calls out the per-command behaviour section as the one that gets missed, and records that CLAUDE.md itself duplicates facts that live in code — when they disagree, the code wins.
Adding ./crux-client to package.json exports followed the letter of the "new lib modules" rule, but it was the wrong call: crux-client is an implementation detail of crux and crux-history, and a subpath entry is a semver commitment to a factory nobody should import. Removed from exports. It stays in tsconfig.types.json, which is not optional — crux.d.ts and crux-history.d.ts reference CruxFormFactor from it, so consumers of web-perf-cli/crux need that declaration to resolve. Verified against a package consumer: web-perf-cli/crux, /crux-history and the root still import, /crux-client now fails with ERR_PACKAGE_PATH_NOT_EXPORTED, and a strict tsc build under node16 resolution still types CruxReport, CruxHistoryReport and the CruxFormFactor union that comes from the private module. The rule is rewritten to separate the two steps. tsconfig include is unconditional, because a private module still needs a .d.ts when published types point at it. package.json exports is a judgement call that decides whether a module is public at all, and internal plumbing stays out of it and out of the README API table.
…constant is live createCruxClient hard-coded CRUX_MAX_REQUESTS_PER_SECOND, which left crux-history's exported CRUX_HISTORY_MAX_REQUESTS_PER_SECOND declared, published through lib/index.js and the types, and read by nothing. Both values are 2.5 today so no run behaved differently, but editing the history constant would have changed nothing — the endpoints could diverge and the module would keep firing at the crux rate. maxRequestsPerSecond is now part of CruxClientConfig, defaulting to CRUX_MAX_REQUESTS_PER_SECOND, and each module passes the constant it publishes. Verified by stubbing runBatch: a client configured at 1.25 hands 1.25 to the rate limiter.
…tate its methods Two related gaps from reviewing the factory. The option typedefs lived in crux.js, so crux-history.d.ts resolved CruxAuditOptions, CruxRunOptions and CruxBatchOptions through the crux subpath — one consumer module's public contract depending on its sibling's. They now sit in crux-client.js alongside CruxFormFactor, which is where the shared behaviour already is, and both modules re-declare them from there. The four factory methods carried no @PARAM, so tsc inferred their options from the defaulted properties only: formFactor vanished from runAudit's type, onNoData from run's, onProgress from both batch methods, and formFactors was typed string[] rather than CruxFormFactor[]. They are now annotated with the shared typedefs and the emitted declarations carry the real shapes. The record return type is still erased by the computed [periodKey]; that is what the typed wrappers in crux.js and crux-history.js exist to restate. Public crux/crux-history types re-checked against a consumer project under node16 resolution.
The refactor moved every real behaviour into createCruxClient but left the configuration itself untested. crux.test.js and crux-history.test.js only reach the client transitively, so swapping endpoint, command, dataLabel or periodKey between the two modules wrote the wrong record into the wrong directory with all 544 tests still green. Adds lib/crux-client.test.js: 13 tests driving the factory through a synthetic config, plus 4 asserting that crux and crux-history are wired to different endpoints, directories, labels and period keys. Confirmed load-bearing by mutation: each of the four config values was swapped in crux-history.js in turn, and each swap fails exactly one test. periodKey plural->singular -> 1 failed | 16 passed results dir -> crux -> 1 failed | 16 passed dataLabel -> CrUX -> 1 failed | 16 passed endpoint -> queryRecord -> 1 failed | 16 passed
Unifying the two clean implementations moved `require('../lib/prompts')` to the top of
cleanAction, putting it on the `web-perf clean <path>` path as well. The subcommand declares
`<input>` as required, so commander rejects the call before the action runs and only the
wizard ever reaches promptClean — the require was unreachable work that also pulled in crux,
crux-client, psi and profiles.
Moved into the branch that needs it, matching the per-branch lazy requires the rest of this
file and lib/lab.js already use. Verified: a non-interactive clean run now loads none of those
four modules, and a missing input still exits 1.
The comment claimed "both filters run over the same predicate in the same order", but they are different predicates over different arrays — `succeeded.filter((_, i) => scores[i] !== null)` and `scores.filter((score) => score !== null)`. The invariant is real and the code is correct; the stated reason was not. What actually holds it up is that `scores` is index-parallel to `succeeded` and both filters select on the same null test while preserving order. Reworded to say that, and to name the consequence of breaking it: medianRun and medianOutputPath silently point at the wrong run.
Owner
Author
|
Review findings addressed in five commits. Attribution was checked against Fixed here (introduced by this PR):
Moved to #16 (pre-existing, not this PR): the One finding was incorrect. It reported that the form-factor arrays became a shared mutable instance in this PR — "previously each module owned its own array literal". They did not:
Verification: lint 0 problems, 561 tests passing (up from 544), types regenerated with no drift, crux/crux-history examples re-run green against the working tree, and the public |
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 #11.
Structural cleanup, no behaviour changes. Seven commits, each independently revertable.
No behaviour change
Every commit here is intended to be observationally identical. Where that is easy to prove,
it is proven below rather than asserted.
withCatchwas dead for six of seven actions. Each action ended with a catch that loggedError: ${err.message}and exited 1, and was also wrapped inwithCatch, which doesbyte-identically the same thing — so the inner handler could never behave differently from
the outer one. Removed from all seven.
One
cleanimplementation instead of two. The wizard'scleanActionand the subcommand'sinline action were the same seven lines twice, differing only in where the input came from.
cleanActionnow takes it as an optional argument and prompts only when absent.cruxandcrux-historycollapsed into one client. The two modules held four functionseach differing only in endpoint, results directory, error label, and
collectionPeriodvscollectionPeriods. Origin normalisation, 404-as-no-data, retry, rate limiting and filenaming were duplicated line for line — which is why the fix in #9 had to be written twice.
createCruxClientin the newlib/crux-client.jsowns that logic once.Free-rider cleanups.
LabReport.timingmarked optional (stripJsonPropsdeletes it bydefault, so the published
.d.tspromised a field the default path removes); each run'sperformance score computed once in
buildRunSummaryinstead of three times; theresults.pushthat an aborting plan throws away removed; over-long JSDoc lines expanded.On the crux refactor
It is not a line-count win. 365 lines became 355. The win is that roughly 250 lines of
duplicated logic became 185 lines that exist once, so the next fix to that path lands in
one place instead of two.
The typed wrappers in
crux.js/crux-history.jsare not ceremony. The factory buildsits record with a computed
[periodKey], which erases the concrete return type to{ [x: string]: any }. I checked the generated.d.tsbefore committing: without thewrappers the published types degrade. With them,
runCruxAuditstill returnsPromise<CruxReport>andrunCruxHistoryAuditstill returnsPromise<CruxHistoryReport>,and the erased shape stays confined to the internal
crux-client.d.ts.The options types are now named (
CruxAuditOptions,CruxRunOptions,CruxBatchOptions)rather than repeated inline. Same shapes — and the regenerated
crux/crux-historytypespick up the
statusCodeargument ononProgressthat the checked-in.d.tswas missing,the same drift class as the
runBatchone corrected in #13.lib/crux-client.jsis registered intsconfig.types.jsonandpackage.jsonexports perthe project checklist.
Verification
npm run lint— 0 problems, down from 6 warnings onmain. The over-long JSDoc linesin
crux.js,crux-history.jsandlab.jsare gone.npm test— 544 passed, unchanged. The crux suite (43 tests) passes untouched, which isthe point: the tests describe the public contract and none of them needed editing.
npm run generate-types— committed; re-running leaves the tree clean../crux,./crux-historyand throughlib/index.js.source, scope, formFactor, url, <period>, extractedAt, metrics, key.https://www.google.com, both endpoints, exit 0:12 metrics on each, 25 collection periods on the history record.
Deliberately out of scope
The
--concurrency=abcvalidation inconsistency (silently falls back to 5, while--runs=abcthrows). It is a behaviour change and does not belong in a no-behaviour-change PR.