ReviewOS

loghqorg/loghq

CI comment tells the next person --lib takes a single file; repeating it works now

#16
Closed glennmichael123 opened this 9 days ago ·
9 days ago

.github/workflows/ci.yml carries the migration plan for making the advisory .stx typecheck load-bearing. One instruction in it is no longer true, and it is the kind that produces a worse config than necessary.

The comment

# This mirrors how bughq got there: warnings-only first, then flipped to
# load-bearing once the count reached 0. Write types/domain.d.ts, drive
# the count down, then delete `continue-on-error` and add the grep that
# fails on anything but "0 error(s), 0 warning(s)".
#
# When that happens, note that --lib must be a SINGLE file: the checker
# does not read ambient types from tsconfig `include`, and repeating the
# flag silently drops all but the last (stacksjs/stx#1926).

The plan is good and nothing else here is wrong. But the last paragraph is an instruction aimed squarely at whoever picks this up, and following it means cramming every ambient declaration into one file when that is no longer required.

It is fixed

Tested against this repo's installed stx, 0.2.532:

probe.stx:
  const one: string = alphaGlobal   // declared in a.d.ts
  const two: number = betaGlobal    // declared in b.d.ts

--lib a.d.ts --lib b.d.ts   ->  0 error(s), 0 warning(s)
--lib a.d.ts                ->  1 error(s)   Cannot find name 'betaGlobal'

The second run is the control and it had to come out red — without it, a checker that failed to launch would look identical to a passing one.

Also confirmed at 0.2.520 (bughq's older version, so this is not a "newer stx only" fix), and stacksjs/analyticshq runs stx typecheck with six --lib flags in CI and passes.

Suggested change

  • Correct the paragraph: --lib is still required, because the checker does not read ambient types from tsconfig include — but it may be repeated, and stacksjs/stx#1926 is fixed.

That is the whole change. Nothing about the migration plan itself needs to move.

Worth knowing for whoever does the migration

The current count is 91 errors, 2 warnings, and 49 of the errors are TS2304 "cannot find name", as the comment says. Two notes from working against it today on loghqorg/loghq#15:

  • The raw count is useless for judging a change while the step is advisory. Build a baseline from origin/main in a throwaway worktree and diff — 91 before, 97 after told me immediately that a change of mine had added six; the number 97 on its own would not have.
  • One class worth expecting: a server-block const and a client-block signal sharing a name. The template resolves the server value, so thing() reports "Type 'Boolean' has no call signatures". keyFull/keyMask avoid it by bridging under initialKeyFull/initialKeyMask, and the note above defineClientPayload already explains why. It is likely a chunk of the 91.

The same issue is filed on bughq as bughq/bughq#21, where the stale line additionally justifies a design decision.

Sign in to comment on this issue.