feat(scripts): generate typed JS API contract and type-check with tsc --checkJs (#1097, #1098) - #1104
Merged
Merged
Conversation
The browser side used to hand-duplicate every action name and the full ADMIN_* permission table from PHP, which made silent drift (a renamed handler / a new permission flag) easy and quietly dangerous. This adds a deterministic generator (`composer api-contract` -> `web/bin/generate-api-contract.php`) that reads `_register.php` and `configs/permissions/web.json` and writes `web/scripts/api-contract.js` with `Actions.*` and `Perms.*` objects, plus best-effort JSDoc typedefs for handlers that already document their @param/@return shapes. The panel layouts now load the contract before sourcebans.js, every sb.api.call('action.name', ...) call site has been migrated to `sb.api.call(Actions.PascalName, ...)`, and the hand-rolled ADMIN_* block at the top of sourcebans.js is gone. A new CI workflow runs the generator on a fresh checkout and fails when the committed file is stale, so the duplication can never come back.
) The panel ships plain JS to the browser, but with no compiler in the loop the JSDoc on sb.* / sourcebans.js was decorative and silent drift was trivial. This wires up tsc --checkJs over web/scripts as a CI gate so mistakes — wrong action shapes, null deref on sb.$id, accidental any — fail before merge. The browser payload is unchanged: tsc only emits diagnostics, nothing in node_modules ships, and the same `// @ts-check` + JSDoc annotations double as IDE hovers. Adds web/package.json + web/scripts/tsconfig.json (strict, checkJs, noImplicitAny, strictNullChecks), web/scripts/globals.d.ts to model the script-tag globals (sb, $, contextMenoo, AddContextMenu, accordion) and the SbAnyEl / SbApiEnvelope shapes the legacy code reads through, and a new sb.$idRequired() helper for the common "I just rendered this id" case. Every existing call site that read .value/.checked off sb.$id() was migrated to either narrow against null or use $idRequired so the strict-null pass stays green. Wires it up end to end: - ./sbpp.sh ts-check runs the gate inside the web container, lazy- installing TypeScript on first use. - docker/Dockerfile gains nodejs + npm so the container has tsc without a separate toolchain. - .github/workflows/ts-check.yml runs the same gate on every PR touching web/** with `npm ci` against the committed lockfile. - docker/README.md documents the new gate alongside phpstan/test. Sequenced after #1097 — Actions.* / Perms.* from the generated contract are what the type-checker now enforces at every call site.
- Migrate the two remaining `sb.api.call('...')` literal sites in
`web/pages/admin.bans.php` and `web/pages/admin.comms.php` to
`Actions.BansAdd` / `Actions.CommsAdd`. The standard header.tpl already
loads `api-contract.js` first, so `Actions.*` is a global on every page
reaching these inline scripts.
- Update the `SbAnyEl` doc in `globals.d.ts` to honestly describe the
trade-off: form members are declared REQUIRED so legacy call sites
compile, at the cost of letting `sb.$id('div').value` type-check even
when the runtime value is `undefined`. Comment now flags the bug-hiding
hazard and points at the typed-helper follow-up.
- Add a banner note to the generated `api-contract.js` (via the
generator) so readers know most `ApiXxxRequest`/`Response` typedefs are
intentional `Object` placeholders pending per-handler docblocks.
- Tighten the cross-reference comment at the bottom of `sourcebans.js`
now that the page-local `ProcessBan()`s actually use `Actions.*`.
rumblefrog
force-pushed
the
js-typing-and-api-contract
branch
from
May 4, 2026 00:30
faca49a to
62158eb
Compare
The two gates added in this PR change the day-to-day dev loop, so the LLM-facing guide needs to reflect them. Adds the new sbpp.sh subcommands to the TL;DR, a `tsc --checkJs` blurb to Quality gates alongside the existing phpstan/test entries, and two Conventions bullets covering the api-contract.js regen workflow (checked-in like a lockfile, CI fails on diff, self-hosters never run codegen) and the // @ts-check / SbAnyEl expectations for JS under web/scripts.
12 tasks
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 #1097, closes #1098. Sequenced as one PR because #1098 imports the
Actions/Permsexports produced by #1097 — splitting them would requiretwo CI passes' worth of churn for no gain.
Summary
#1097 — Generate typed JS API contract from PHP handler registry
web/bin/generate-api-contract.phpparsesweb/api/handlers/_register.phpfor action names + permission masks, reads
web/configs/permissions/web.jsonfor the canonical
ADMIN_*constants, and emitsweb/scripts/api-contract.jswith
ActionsandPermsglobals plus@typedefs for every action. Outputis alphabetically sorted and byte-stable across runs.
composer api-contractand into.github/workflows/api-contract.yml,which fails CI if the file is out of date.
sb.api.call('foo.bar', …)literal inweb/scripts/,web/themes/,AND
web/pages/is migrated toActions.PascalName. The hand-rolledADMIN_*block at the top ofsourcebans.jsis gone; references gothrough
Perms.*.#1098 — Type-check vanilla JS with
tsc --checkJsand JSDocweb/package.json(TypeScript dev-only — nothing ships fromnode_modules/) +web/scripts/tsconfig.jsonwithcheckJs,noEmit,strict,strictNullChecks..jsunderweb/scripts/carries// @ts-check. JSDoc annotationson
sb.*andsb.api.*exported surface; ambient declarations inweb/scripts/globals.d.tsfor the cross-file shapes.sb.$idRequired(id)helper for "missing element is a programmererror" call sites;
sb.$id()callers that handle absence narrow withif (!el) return;../sbpp.sh ts-checkrunstsc --noEmitinside the web container; new.github/workflows/ts-check.ymlruns the same gate in CI on every PR.docker/Dockerfilegainsnodejs npm(Debian 12 ships Node 20).docker/README.mddocuments the gate alongsidephpstanandtest.Review-cycle fixups (third commit)
A reviewer pass surfaced three nits, all addressed before push:
sb.api.call('...')literals inweb/pages/admin.bans.phpand
admin.comms.phpmigrated toActions.*.SbAnyEldoc updated to honestly describe the legacy/strict trade-off(form members declared REQUIRED to keep call sites compiling, with a
flagged hazard and a pointer to a typed-helper follow-up).
api-contract.jsbanner now calls out that mostApi…Request/
…Responsetypedefs are intentionalObjectplaceholders pendingper-handler docblocks.
Test plan
./sbpp.sh composer api-contractruns cleanly; second run producesa byte-identical file (verified with
diff /tmp/snapshot api-contract.js)../sbpp.sh phpstan—[OK] No errors../sbpp.sh test—OK (37 tests, 99 assertions)../sbpp.sh ts-check— exit 0, zero diagnostics.rg "sb\.api\.call\(\s*['\"]" web/— no matches anywhere inweb/.rg "^\s*(const|var|let)\s+ADMIN_" web/scripts/— no matches.web/scripts/*.jscarry// @ts-check.node_modules/ignored, not tracked.as admin, exercise an admin-page action that round-trips through
sb.api.call, confirm no console errors.Known gaps a reviewer should look at
Objectplaceholders. Tighteningthem is gated on adding
@param/@returndocblocks to the handlerbodies — left as a follow-up so this PR doesn't sprawl.
SbApiEnvelope.datais typedanyfor the same reason; converting itto a discriminated union keyed off
Actions.*is the natural follow-uponce handler docblocks land.
SbAnyElis intentionally permissive (see the doc comment). New codeshould prefer typed selectors; a follow-up issue could introduce
sb.$input()/sb.$select()helpers and migrate call sites.