Ship the CLI as TypeScript and build out the test suite - #142
Merged
Conversation
The CLI was the last JavaScript left in the project. It is TypeScript now, and it ships that way: `bin` points straight at the .ts sources and Node strips the types as it loads them, so what you read in cli/ is exactly what npm installs -- nothing is compiled, bundled or minified on the way. tsc never emits, it only type-checks. That costs a Node floor. Type stripping is unflagged from 22.18 on and genuinely absent before it -- 22.17 cannot load a .ts file at all -- so `engines` is >=22.18 and this is a breaking change, hence 0.3.0. The floor being at patch level is deliberate rather than a typo, and is spelled out in the README, the CI matrix and tools/Dockerfile. `erasableSyntaxOnly` keeps the source to constructs that can actually be erased: no enums, namespaces or constructor parameter properties, all of which need real codegen and would fail at a user's runtime rather than in CI. The bins are now thin shims over cli/lib. That is not tidying: the old entry points were top-level scripts that a test could only exercise by spawning a process, and V8 coverage cannot see into one, so they would have scored zero however well tested they were. The logic moved into modules that a test can import, and the argv parsing, the UDP client, the stream store and the server wiring are each testable on their own. Tests come in three layers. test/unit covers the CLI modules, the webapp's libraries and all five Preact components against jsdom; test/integration runs the real bins as processes over a real UDP socket, which is the only thing that proves the published .ts actually executes; test/e2e drives the built webapp in Chromium, fed by the real client bin. 238 node:test cases and 15 Playwright ones, and the run now fails below 90% line, branch and function coverage. It currently sits at 99.8/95.8/97.0. Node cannot load .tsx at any version -- stripping is an erasure pass and JSX needs a real transform -- so tools/tsx-hook.ts runs the components through esbuild on the way into the test runner, reusing the JSX settings from tools/build.ts so a component cannot behave differently under test than in the browser. Source maps are inline, so coverage still reports against the original .tsx lines. Chromium is heavy enough to deserve its own image, so it lives in tools/Dockerfile.e2e behind `make test-e2e` and a separate CI job, and the Playwright version is pinned exactly in both places because each release expects a specific browser revision. Two fixes fell out of writing this. StreamView attached its wheel-to-pause listener in an effect keyed on [onPause], which ran once against a null ref while no stream was selected and never again once the scroller appeared -- so scrolling to pause did nothing at all for anyone who picked a stream from the empty state, and worked only if one was already selected at load. And closing the server while a long-poll was in flight left engine.io's grace timer armed, which held the process open and made the suite hang roughly one run in five; shutdown now waits for the disconnect and sweeps what survives. One operational note: two tests assert the built webapp is served, so `npm run dist` has to precede `npm test`. CI and the make targets do it.
#141 landed the query language and field extraction while this branch was converting the CLI to TypeScript and building the test suite. The two touch the same files, so this is a real merge rather than a fast-forward. The conflicts themselves were only import blocks -- main added imports to the same lines this branch had rewritten to explicit .ts specifiers -- and main's side won on content, with the extensions reapplied. Three things needed more than that. test/query.test.js moved to test/unit/query.test.ts. Left where it was it would have matched none of the new globs and silently stopped running. It also bundled the module into a data: URL with esbuild, a workaround for node:test not being able to import TypeScript; that is no longer true, and the direct import matters beyond tidiness, because coverage cannot attribute a data: URL back to app/src/lib/query.ts -- the bundled version scored the module at zero however much it exercised. The ~800 lines main added are now covered: query.ts, json.ts, highlight.ts and SearchBar.tsx, plus format.ts's renderFields and defaultExpanded and prefs' jsonView and per-stream fields. The existing tests moved to the new APIs -- buildFilter is gone, StreamView no longer filters, and Line and Prefs both grew fields. 402 tests, and the 90% gate holds on the merged tree rather than on a shrunken surface. Two fixes carried over. The scroll-to-pause bug was reintroduced by main's rewrite of StreamView -- the wheel listener keyed on [onPause] again -- so the fix is reapplied, and the e2e case for it now reaches the stream through the sidebar, which is the path that was broken. And highlight() threw on a needle whose regexp already carried `g`, since it appends `g` unconditionally; unreachable through parseQuery, which strips stateful flags, but it is an exported helper and the fix is one call. The e2e suite follows the feature: field filtering, the syntax card, marking hits, field extraction, and payload collapsing, 21 cases in all.
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.
The CLI was the last JavaScript left in the project. It is TypeScript now, and it ships that way — no build step, and a test suite that actually covers it.
Shipping TypeScript without compiling
binpoints straight at the.tssources and Node strips the types as it loads them, so what you read incli/is exactly what npm installs.tscnever emits; it only type-checks.That costs a Node floor, and the boundary is sharp:
node a.tsSo
enginesis>=22.18. This is a breaking change — hence the bump to 0.3.0. The floor sitting at patch level is deliberate rather than a typo, and is spelled out in the README, the CI matrix comment andtools/Dockerfile.erasableSyntaxOnlykeeps the source to constructs that can genuinely be erased — no enums, namespaces or constructor parameter properties, all of which need real codegen and would fail at a user's runtime rather than in CI.Compiling was considered and rejected: once Node 20 is out of scope either way, the only remaining gap is Node 22 installs more than a year behind on patches, which is not worth a permanent build step and the class of "works from source, broken from the tarball" bugs that cannot exist today.
The bins are now thin shims
Not tidying. The old entry points were top-level scripts that a test could only exercise by spawning a process, and V8 coverage cannot see into one — they would have scored 0% however well tested they were. The logic moved into
cli/lib, so argv parsing, the UDP client, the stream store and the server wiring are each importable and testable on their own.Three layers of tests
test/unittest/integrationtest/e2eThe run fails below 90% line, branch and function coverage. It currently sits at 99.8 / 95.8 / 97.0.
Node cannot load
.tsxat any version — stripping is erasure and JSX needs a real transform — sotools/tsx-hook.tsruns components through esbuild on the way into the test runner, reusing the JSX settings fromtools/build.tsso a component cannot behave differently under test than in the browser. Source maps are inline, so coverage still reports against the original.tsxlines.Chromium is heavy enough to deserve its own image:
tools/Dockerfile.e2e, behindmake test-e2eand a separate CI job. The Playwright version is pinned exactly in both places, because each release expects a specific browser revision.Two fixes that fell out of writing this
Scroll-to-pause was broken in production.
StreamViewattached its wheel listener in an effect keyed on[onPause]. It ran once against a null ref while no stream was selected, and never re-ran once the scroller appeared — so scrolling to pause did nothing for anyone who picked a stream from the empty state. It only worked if a stream was already selected at load, via deep link or the remembered stream, which is why it went unnoticed. Guarded now at both the unit and browser level; the regression test was verified by reverting the fix and watching it fail.The suite hung about one run in five. Closing the server while a long-poll was in flight left
engine.io's grace timer armed, holding the process open. Tracked down by dumping async-hook creation stacks rather than papering over it with--test-force-exit; shutdown now waits for the disconnect and sweeps what survives.Notes for the reviewer
npm run distmust precedenpm test— two tests assert the built webapp is served. CI and the make targets handle it; a barenpm teston a fresh clone will not.app/src/main.tsxis the one coverage exclusion: an 11-line bootstrap, exercised for real by the e2e suite, which loads the actual bundle.Verified before pushing: typecheck clean, 238 unit/integration passing, 15 e2e passing across repeated runs, runtime Docker image builds and serves, and
make devstill works.🤖 Generated with Claude Code