Drop the dead dist emit and the duplicated type check - #163
Conversation
github-actions
Bot
added
configurator
|
The PR is well-constructed. Let me summarize my review. Review SummaryThis is a well-executed build-performance PR. I verified the changes across all 76 files and found the mechanics sound: Correctness checks that pass:
Non-blocking notes (not requiring changes):
LGTM! |
This comment was marked as outdated.
This comment was marked as outdated.
2ce778d to
ae58e17
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
2e72902 to
8cd8c88
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
No I've completed my independent review. The mechanics are sound and thoroughly covered. My verification confirms:
LGTM! |
8cd8c88 to
70308a0
Compare
|
Dependencies aren't installed and the working tree is read-only, so I can't run a full build. I've verified the mechanics statically across the changed files. My independent review is complete. Review SummaryI independently verified the key mechanics of this build-performance PR against the actual source tree:
One minor factual correction to an earlier automated comment (non-blocking, no code impact): the claim that No actionable issues found. LGTM! |
e27d172 to
70308a0
Compare
|
Catalog versions in the lockfile match the previously-pinned versions (typescript 5.9.3, vitest 4.1.10, vitest-pool-workers 0.18.8, and vite 7.3.6/wrangler 4.119.0 from the workspace file). The lockfile is consistent. I've completed a thorough independent review. Everything checks out. Review SummaryThis is a well-constructed build-performance PR. I verified the mechanics statically against the source tree (dependencies aren't installed and the tree is read-only, so I could not run the build). Verified correct:
Note on prior automated comments: Several earlier No actionable issues found. LGTM! |
…place tsc's Root `build`, `test` and `clean` become `vp run -r <task>` instead of `pnpm run --recursive`. Vite+ runs the same per-package scripts in the same dependency order, but schedules them better and caches each against its inputs, so an unchanged package replays its previous result. Commands joined with `&&` cache as separate entries, so a package with fresh codegen can still re-run its `tsc`. Note the root script cannot stay `pnpm run --recursive`: `vp run -r` selects the workspace root as a package too, so it would run that script as a task and rebuild everything a second time. Vite+ prunes the self-reference only when the root task is itself `vp run`. Removes `incremental`/`tsBuildInfoFile` from the package tsconfigs, which PR #163 added. The two caches were mutually exclusive, not additive: Vite+ refuses to cache a task that reads a path it also writes, and `tsc` reads its own `.tsbuildinfo` and writes it back. Keeping `incremental` held the whole type check out of the task cache to save strictly less than the task cache saves. Measured on a clean tree with a warm cache -- CI's shape -- that is 2% cache hits and 21.1s against 65% and 13.4s. Local warm builds are unchanged either way (11.6s vs 11.7s), so nothing is traded away. Timings, all on the same machine against this tree, `pnpm build`: before after cold, clean tree, no cache 33.5s 21.8s clean tree, warm cache (CI) 33.5s 14.0s warm, steady state 24.7s 12.0s pnpm test 41.0s 33.5s CI caches `node_modules/.vite/task-cache` keyed per run, restoring the newest earlier entry, so the 14.0s column is what PR builds should see once one run has populated it. Three tasks never cache: the `vite build` SPA bundles in workshop-frontend, gatekeeper-context and gatekeeper-scheduler, each of which reads a file it also writes. Tests mostly don't cache for the same reason, so their gain is scheduling rather than replay. `pnpm lint`, `pnpm build`, `pnpm test` and workshop-backend's `test:integration` are green from a clean tree. Co-Authored-By: Claude Opus 5 <[email protected]>
…place tsc's Root `build`, `test` and `clean` become `vp run -r <task>` instead of `pnpm run --recursive`. Vite+ runs the same per-package scripts in the same dependency order, but schedules them better and caches each against its inputs, so an unchanged package replays its previous result. Commands joined with `&&` cache as separate entries, so a package with fresh codegen can still re-run its `tsc`. Note the root script cannot stay `pnpm run --recursive`: `vp run -r` selects the workspace root as a package too, so it would run that script as a task and rebuild everything a second time. Vite+ prunes the self-reference only when the root task is itself `vp run`. Removes `incremental`/`tsBuildInfoFile` from the package tsconfigs, which PR #163 added. The two caches were mutually exclusive, not additive: Vite+ refuses to cache a task that reads a path it also writes, and `tsc` reads its own `.tsbuildinfo` and writes it back. Keeping `incremental` held the whole type check out of the task cache to save strictly less than the task cache saves. Measured on a clean tree with a warm cache, that is 2% cache hits and 21.1s against 65% and 13.4s. Local warm builds are unchanged either way (11.6s vs 11.7s), so nothing is traded away. Timings, all on the same machine against this tree, `pnpm build`: before after cold, clean tree, no cache 33.5s 21.8s clean tree, warm cache 33.5s 14.0s warm, steady state 24.7s 12.0s pnpm test 41.0s 33.5s CI is left alone: it gets the cold column, 33.5s to 21.8s from scheduling alone, with no cache to restore or store. Persisting the task cache between runs is a separate question with its own tradeoffs, and can be revisited once this has run for a while. Three tasks never cache: the `vite build` SPA bundles in workshop-frontend, gatekeeper-context and gatekeeper-scheduler, each of which reads a file it also writes. Tests mostly don't cache for the same reason, so their gain is scheduling rather than replay. `pnpm lint`, `pnpm build`, `pnpm test` and workshop-backend's `test:integration` are green from a clean tree. Co-Authored-By: Claude Opus 5 <[email protected]>
…place tsc's Root `build`, `test` and `clean` become `vp run -r <task>` instead of `pnpm run --recursive`. Vite+ runs the same per-package scripts in the same dependency order, but schedules them better and caches each against its inputs, so an unchanged package replays its previous result. Commands joined with `&&` cache as separate entries, so a package with fresh codegen can still re-run its `tsc`. Note the root script cannot stay `pnpm run --recursive`: `vp run -r` selects the workspace root as a package too, so it would run that script as a task and rebuild everything a second time. Vite+ prunes the self-reference only when the root task is itself `vp run`. Removes `incremental`/`tsBuildInfoFile` from the package tsconfigs, which PR #163 added. The two caches were mutually exclusive, not additive: Vite+ refuses to cache a task that reads a path it also writes, and `tsc` reads its own `.tsbuildinfo` and writes it back. Keeping `incremental` held the whole type check out of the task cache to save strictly less than the task cache saves. Measured on a clean tree with a warm cache, that is 2% cache hits and 21.1s against 65% and 13.4s. Local warm builds are unchanged either way (11.6s vs 11.7s), so nothing is traded away. Timings, all on the same machine against this tree, `pnpm build`: before after cold, clean tree, no cache 33.5s 21.8s clean tree, warm cache 33.5s 14.0s warm, steady state 24.7s 12.0s pnpm test 41.0s 33.5s CI is left alone: it gets the cold column, 33.5s to 21.8s from scheduling alone, with no cache to restore or store. Persisting the task cache between runs is a separate question with its own tradeoffs, and can be revisited once this has run for a while. Three tasks never cache: the `vite build` SPA bundles in workshop-frontend, gatekeeper-context and gatekeeper-scheduler, each of which reads a file it also writes. Tests mostly don't cache for the same reason, so their gain is scheduling rather than replay. `pnpm lint`, `pnpm build`, `pnpm test` and workshop-backend's `test:integration` are green from a clean tree. Co-Authored-By: Claude Opus 5 <[email protected]>
70308a0 to
de0f8d5
Compare
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review submitted with 1 inline finding. Review SummaryI independently verified this build-performance PR against the actual source tree. The mechanics that the prior automated reviews covered are indeed sound: the emit-config drops correctly fall back to root One actionable finding (posted inline): The PR body's headline claim of "per-config Notably, every prior This is documentation/comment drift, not a functional defect — the build and type-checks are correct as-is. |
de0f8d5 to
c71df12
Compare
|
All 6 My review is complete. I've independently verified all mechanical claims against the actual source tree at HEAD
Regarding the one open inline finding from a prior automated review (the "incremental cache" comment/PR-body drift): at this HEAD, No actionable code issues found. LGTM! |
Maximo-Guk
changed the title
dist emit and the duplicated type check
`pnpm build` emitted declarations and sourcemaps that nothing reads, the type check ran twice per CI run in two jobs that shared nothing -- once as `lint`'s `types:check` (`tsc --noEmit`), once as `test`'s `build` (`tsc`) -- and CI had no pnpm store cache. Local build time is not what this changes. Measured on an M-series laptop, `time pnpm build`, three runs each way: 35s cold and 32s warm before, 35s cold and 32s warm after. The dead emit is a small share of a build dominated by codegen and the three `vite build` bundles, and nothing here caches across runs. What this buys is a CI run that installs from cache and type-checks once instead of twice, and a tree that the follow-up can put behind Vite+'s task cache -- which is where the wall clock actually moves (35s to 21.8s cold, to 12s warm). Five changes, each independently defensible: **Dead `dist` emit dropped.** Every package but `typed-storage` was emitting declarations and sourcemaps that nothing imports -- wrangler and vite bundle from source, and every `exports` map resolves to `./src/*.ts`. The set was determined mechanically by walking each manifest's `exports` tree, not by hand: only `typed-storage` resolves to `dist/index.js` (`scripts/run-local.mjs` even uses its `dist` as a build sentinel), so it keeps emitting. `configurator-ui` and `workshop-shared` also had vestigial `main`/`types` pointing at the removed output, unreachable behind their own `exports`; those are gone. A visible symptom of the dead emit: `gatekeeper-context/dist` contained `gatekeeper-context/`, `typed-storage/` and `workshop-shared/` subtrees, tsc inferring a `rootDir` above the package because the cross-package source imports pulled siblings into the program. **`build` and `types:check` collapsed.** With nothing emitted they are the same command, so the root `types:check` is now an alias for `build` and the per-package ones are deleted. `build` had to be the survivor, not the other way round: it is where the codegen prerequisites hang, and `scripts/release/build-release.mjs` relies on `pnpm build` having produced `src/generated/*-configurator-ui.txt`. Three packages kept a second tsconfig only to check tests without emitting; with no emit the split had no purpose, so their includes are merged into the base config and the extra configs are deleted (`backend-utils`, `gatekeeper-mcp`, `gatekeeper-mcp-portal`). `mcp-shared` keeps its test config, which also adds `node` types the Worker's own check must not see. Trade-off worth naming: `pnpm lint` now also runs the frontend `vite build` and the two SPA builds, because it is now literally the CI pass rather than an approximation of it. **`build-format-blueprints.mjs` compares before writing.** It ran 3-4x per CI run and rewrote its generated module unconditionally every time, giving it a fresh mtime on every invocation. The other three generators already compared content first; this one is now consistent with them, which is what lets the follow-up's task cache treat a repeat run as a hit. **CI collapsed to one job**, with the pnpm store cached (`cache: pnpm` on GitHub, a keyed `.pnpm-store` on GitLab). `corepack enable` moves ahead of `setup-node`, which shells out to `pnpm store path` to find what to cache. Note for whoever merges: this replaces the `Lint` and `Build and test` checks with a single `Lint, build and test`, so branch protection needs updating. **Toolchain versions in a `pnpm-workspace.yaml` catalog** -- `typescript`, `vitest`, `wrangler`, `vite` and `@cloudflare/vitest-pool-workers` were pinned separately in ~24 manifests. The lockfile diff confirms no resolved version moved; `integration-tests` had drifted to `~4.119.0` for wrangler, which resolved to the same 4.119.0 the rest of the repo was already on. `vite` stays duplicated in `overrides` on purpose, since a catalog only covers what a workspace manifest declares itself and the pin needs to reach vite as somebody else's transitive peer. Verified: `pnpm lint && pnpm build && pnpm test` clean, plus `pnpm --filter @gadgets/workshop-backend test:integration` (unchanged: 1 passed, 4 skipped behind a pre-existing `describe.skip`). oxlint reports 0 errors and 64 warnings, same as before. `pnpm clean` now sweeps the whole tree -- four packages had no `clean` script, so a stale `dist` would have survived it. Groundwork for adopting Vite+ (`vp lint`, per-package `vite.config.ts`, and the task cache that is what finally makes a repeat build cheap). Findings and approach from #33 by @ubugeeei. Co-Authored-By: Claude Opus 5 <[email protected]>
Five packages run their tests inside workerd via @cloudflare/vitest-pool-workers. When that pool fails to start, vitest runs the files under Node instead -- and only the suites that import a `cloudflare:*` module notice. `router` and `typed-storage` import none, `backend-utils` has a single `cloudflare:workers` import in one of three files, so a broken pool leaves them green while testing the wrong runtime entirely. That is not hypothetical: it is how PR #33 came to report a pool it had broken as "passing". Add `test-setup/assert-workerd.ts`, which throws unless `navigator.userAgent` is `Cloudflare-Workers`, and list it in `setupFiles` for all six pool-workers config files. Verified by removing `cloudflareTest` from each config in turn and confirming the suite fails -- previously `router`, `typed-storage` and `backend-utils` passed that same experiment. Findings and approach from #33 by @ubugeeei. Co-Authored-By: Claude Opus 5 <[email protected]>
Partly supersedes db12b8d, which collapsed `lint` and `test` into one job. That removed the duplicated type check but serialized the feedback: a lint failure aborted the job before the tests ran, so you learned about one problem per push. The duplication was never the job split -- it was the `lint` job running `pnpm lint`, which is `lint:check` plus `types:check`, the same recursive `tsc` as the `test` job's `build` step. Running `lint:check` alone in that job removes the second type check and keeps the two jobs parallel, which is strictly better than either previous arrangement. The pnpm store cache from db12b8d makes the second install a cache hit, so the split costs close to nothing. Job names go back to `Lint` and `Build and test`, so branch protection needs no change after all. Also deletes .gitlab-ci.yml: this repo is on GitHub Actions, and everything the pipeline did is already covered there -- lint/build/test by ci.yml, AI review by the opencode component's counterpart in bonk.yml. gadgets-internal stays on GitLab, but it vendors this repo as a submodule and runs `pnpm --dir public build && pnpm --dir public test` from its own pipeline, so it never read this file. `.pnpm-store/` leaves .gitignore with it: it was added in db12b8d only because GitLab can only cache paths inside the project, whereas setup-node's `cache: pnpm` caches the default global store. Co-Authored-By: Claude Opus 5 <[email protected]>
Co-Authored-By: Claude Opus 5 <[email protected]>
c71df12 to
dd5a1ac
Compare
|
Review submitted with 1 inline finding. Review SummaryI independently verified this build-hygiene PR against the source tree at HEAD One actionable finding (posted inline):
I did not reproduce the "PR-body claims per-config incremental caches" finding from the older automated comments as an inline finding, since the PR body is not part of the merged tree; I've folded it into this note instead. The actionable, in-tree instance of that same inaccuracy is the source comment above. |
ndisidore
left a comment
ndisidore
left a comment
There was a problem hiding this comment.
Awesome cleanup initiative!
Trim the job-level comments to one line each per review feedback; keep the corepack-before-setup-node ordering note since it guards a real footgun. Co-Authored-By: Claude Fable 5 <[email protected]>
|
The latest commit Per my guidance, I must not repeat a published finding, and I must not submit a review without a new inline finding. My independent review of all changed files confirms the mechanics are sound and the only actionable issue (the The one open actionable finding on this PR — the
Everything else verified correct: emit-config drops fall back to root I have no new, distinct actionable finding to post inline beyond the one already published, so I am not submitting a duplicate review. LGTM! |
2 participants
This PR implements a few build hygiene improvements for CI + local dev:
types:checkfrom thelintjob so the type check runs once. The two jobs stay parallel, so a lint failure and a test failure surface on the same run.distemit - onlytyped-storagehasexportsresolving todist. Everything else isnoEmit.pnpm-workspace.yamlcatalog instead of being pinned in package.json files..gitlab-ci.yml: this repo is now on GitHub Actions