Fix the docs typecheck script, and gate the docs site on PRs - #141
Merged
Conversation
`docs/package.json` had `"typecheck": "tsc"`, but the workspace catalog maps `typescript` to `npm:@typescript/typescript6`, whose bin is `tsc6`. The script had never run — it failed with `sh: tsc: command not found`, and nothing in `.github/workflows/docs.yml` invoked it, so nobody noticed. Point it at `tsc6` rather than at the TS 7 `@typescript/native` the package build scripts use: `@docusaurus/tsconfig@3.10.2` sets `baseUrl` in its own compilerOptions, and TS 7 removed the option outright (TS5102). An inherited option cannot be unset, so TS 7 here would mean dropping `extends` and inlining Docusaurus's config — no benefit for a `noEmit` check that publishes nothing, and permanent drift from upstream. `tsc6` also matches the catalog's stated split: TS 6 for tooling, `@typescript/native` for declaration emit. With the script running, two real errors surfaced: - `TS7016` on `react` — docs depends on `react` but never declared `@types/react`. - `TS2307` on `@spatialdata/vis` — the workspace packages resolve types through `dist/index.d.ts`, so the typecheck requires `pnpm build` first. Hence the ordering of the new step in the Docs workflow. `baseUrl` has to stay in `docs/tsconfig.json`: it is what anchors the inherited `@site/*` mapping to `docs/` instead of to the `@docusaurus/tsconfig` package directory. TS 6 only deprecates it, so `ignoreDeprecations: "6.0"` covers it until Docusaurus drops it upstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Docs workflow now runs `pnpm --filter docs typecheck`, but it triggers only on push to main, so a type error in the docs site still lands and only fails on the way out to Pages. `pnpm build` and `pnpm test` both filter docs out, so no PR check compiled it at all. Add a `docs-typecheck` job to the Test workflow, which runs on pull_request. It builds the workspace packages first — docs imports `@spatialdata/vis` and the packages resolve types through `dist/index.d.ts`, so the typecheck cannot run against a bare install. Verified with actionlint 1.7.12, the same pin the Workflow Lint job uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The typecheck gate does not compile the site, so broken MDX, a dead internal link, or a Docusaurus config regression still reached main and failed only on the way out to Pages — which is how the 3.10 upgrade was validated. `pnpm docs:build` reuses the `pnpm build` the job already runs, so the marginal cost is the Docusaurus build alone. It runs after the typecheck so the cheap check reports first, matching the Docs workflow's Build job — which this job is now a copy of, minus the deploy. Renamed `docs-typecheck` to `docs` accordingly. The old name never ran, so no branch protection rule can be referencing it yet. Verified with actionlint 1.7.12; both commands run clean locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR updates the docs package for TypeScript 6 and adds docs typecheck and build validation to GitHub Actions workflows. ChangesDocumentation CI validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
docs/package.jsonhad"typecheck": "tsc", but the workspace catalog mapstypescripttonpm:@typescript/typescript6, whose bin istsc6. The script had never once run — it failed withsh: tsc: command not found. Nothing in.github/workflows/docs.ymlinvoked it, which is why that went unnoticed.Pre-existing; unrelated to the Docusaurus 3.10 upgrade in #139, just found next to it.
Why
tsc6and not TS 7@docusaurus/tsconfig@3.10.2setsbaseUrlin its own compilerOptions, and TS 7 removed the option outright:An inherited option can't be unset, only overridden, so using
@typescript/nativehere would mean droppingextendsand inlining Docusaurus's config — a hand-copied snapshot of theirtarget/lib/moduleResolutionthat silently stops tracking upstream, in exchange for nothing on anoEmitcheck that publishes no declarations.tsc6also matches the catalog's stated split: TS 6 for tooling,@typescript/nativefor declaration emit.I did verify TS 7 works: with the config inlined it typechecks the site with zero errors, and
@site/*resolves correctly under paths-without-baseUrl. It's deliberately left for the Docusaurus v4 upgrade, where it costs nothing — upstream removedbaseUrlin facebook/docusaurus#11915 (merged, v4 milestone, not in 3.x), moving the@sitealias and excludes into the base config. At that pointdocs/tsconfig.jsoncollapses to{ "extends": "@docusaurus/tsconfig" }, theignoreDeprecationsline goes away, and the script moves totscwith no local fork of upstream's config at any point.Real errors the working script surfaced
TS7016onreact— docs depends onreactbut never declared@types/react. Added from the catalog. (@types/react-domturned out not to be needed; checked, left out.)TS2307on@spatialdata/vis— the workspace packages resolve types throughdist/index.d.ts, so the typecheck needspnpm buildfirst. That's why the new steps are ordered the way they are, and it is not incidental: on a bare install the check cannot run at all.baseUrlhas to stay indocs/tsconfig.json— it is what anchors the inherited@site/*mapping todocs/rather than to the@docusaurus/tsconfigpackage directory. TS 6 only deprecates it, soignoreDeprecations: "6.0"covers the gap.Gating
The Docs workflow triggers only on push to main, so a failure there is already blocking a Pages deploy by the time anyone sees it — and
pnpm buildandpnpm testboth filter docs out, so no PR check compiled the site at all. That is how the 3.10 upgrade got validated.New
docsjob in the Test workflow, which runs onpull_request. It is the Docs workflow's Build job minus the deploy: install →pnpm build→ typecheck →pnpm docs:build. The site build catches what the typecheck cannot — broken MDX, dead internal links, Docusaurus config and plugin regressions — and reuses thepnpm buildalready in the job, so the marginal cost is the Docusaurus build alone. Typecheck runs first so the cheap check reports first.Verification
pnpm --filter docs typecheck— exit 0pnpm docs:build— exit 0,[SUCCESS] Generated static filesThe one warning in the docs build output is the pre-existing
web-workercritical-dependency notice, unchanged by any of this.Note for whoever merges
If branch protection lists required checks by name, the new
docscheck won't be required until it's added to that set — it will run and can fail without blocking merge. That's a repo-settings change, not something in this diff.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores