fix: scaffolded apps get real core types and a usable typescript range - #1452
Conversation
|
Design note: why the guard detects the cause instead of the My first cut of The reason is worth writing down, because the obvious detector is the wrong one here. An export that resolves through a missing What does discriminate is the cause: run tsc the way an app resolves the package, with The headline test keeps a type-level check but probes by ASSIGNMENT, to a branded type nothing real inhabits. A real type errors; Why the guard pins
The scaffold change was not planned, and is the interesting part Fixing |
|
Decision: why the range went to The bug only requires clearing 5.8, so I checked the range is not just newer but correct, by generating both templates and type-checking them under a real 6.0.3: clean on both. I had also checked 7.0.2 earlier while chasing the original report, also clean, so the eventual move to 7 is a range bump rather than a migration. Why the guard reads a table instead of just asserting a constant The simplest guard would be Both halves are proven by toggling: restoring The docs sample was independently wrong
I did not add a It was tempting, since doctor already knows about |
`@webjsdev/core`'s overlay re-exported seven modules from their JSDoc `.js` with no `.d.ts` sibling. An app has `allowJs` off, so `html`, `css`, `TemplateResult`, `Suspense`, `repeat`, `connectWS`, `richFetch` and the escape helpers all resolved to `any` there, silenced by `skipLibCheck`. That took a component's `render()` return, its `static styles` and a page's return type with them, so a scaffolded app type-checked almost none of its templates. `@webjsdev/server` had the same class of break in one spot: `RequestHandler` referenced `Handle` on the strength of an `export *`, which re-exports a name without binding it locally, so `handle` was an error type. Fixing it exposed a real too-narrow signature in the gallery's rate-limit test, which now derives the type from `Handle` instead of restating it. The two existing drift guards could not see any of this: both run tsc with `--allowJs`, which reads the JSDoc the app never gets. The new guard inverts that flag and `skipLibCheck` so it grades the packages the way an app does.
The generated package.json declared `"typescript": "^5.6.0"` while the tsconfig.json the same generator writes sets `erasableSyntaxOnly`, which landed in 5.8. Every version in the lower half of that range refuses the config outright with `TS5023: Unknown compiler option`, exit 2, nothing else checked. It stayed hidden because npm resolves a caret to the newest match, so a fresh scaffold picked up 5.9; it bites a pinned install, an older lockfile, or an editor whose own compiler is older. The range moves to the major the repo's own three apps already use, so an app and the framework that generated it type-check under one compiler. Both templates were generated and type-checked clean under 6.0.3. Nothing tied the two files together, so a new guard does: it maps every compiler option the generator emits to the release that introduced it and asserts the range's LOWEST version clears the highest of those floors. An option missing from the table fails the test rather than being skipped, so adding one has to record its floor. The docs site showed `^5.7.0` in its api-template manifest, itself below the floor, so that sample is corrected too.
The generated tsconfig is plain JSON.stringify output with no comments; the comment-stripping is defensive, not a present need. Say so.
a3cc71f to
20335c2
Compare
vivek7405
left a comment
There was a problem hiding this comment.
I went over this end to end: each new overlay against the runtime module it declares, both index files, the two guards, and the generator and docs constants.
The overlays are honest. Export sets match their .js exactly in both directions, including SUSPENSE, which is easy to miss because index.d.ts does not re-export it. Two places refine on the JSDoc rather than copy it, isRepeat / isSuspense as type predicates and connectWS's onMessage payload as unknown, which is the divergence the phantom guard's own header calls out as deliberate house style, so I am happy with both. MARKER keeps its literal type and its value is untouched, which matters given #730.
The part I like most is that the new guard detects the CAUSE rather than the symptom. A type-level any sweep would have been the obvious build and it would have been dead on arrival, because an unresolved import is the error type and that absorbs conditionals. Inverting allowJs and skipLibCheck and reading TS7016 is the thing that actually discriminates, and pinning paths at this checkout is what stops it grading the primary through the worktree symlink. Both are the sort of decision that looks arbitrary in six months, so I am glad the reasoning is written on the PR.
One real problem, on the floor guard's comment, flagged inline and already fixed. One thing I looked at and decided to leave, also inline.
Main moved under this branch while I was reading, so I rebased onto #1450 and re-ran both guards on the new head.
Typing RequestHandler.handle for real broke every place that had restated it as (req: Request) => Promise<Response>. Those only passed while Handle was an unbound name, so the error type absorbed the mismatch: the website's four SSR test wrappers, and the server export fixture. The wrappers become async, the fixture states the real union. connectWS's onMessage payload goes back to `any`, matching its JSDoc. Refining it to `unknown` broke the blog's chat and comments handlers, which name the message shape they expect. That is the contract, and a PR filling in missing declarations does not get to change it. The new no-any guard joins the bun denylist beside its #1031 sibling: it spawns process.execPath as Node tsc, so under the matrix it spawns bun and every probe reads as `any`. Local runs missed all of this because a linked worktree resolves bare @webjsdev/* to the PRIMARY checkout, so the tests graded an unfixed copy. Verified by shadowing the packages into each test tree, which reproduced CI exactly.
The no-explicit-any suppression I put on connectWS's onMessage suppresses nothing here: this repo has no eslint config and no lint script. The comment above it already carries the reason the `any` is deliberate.
Closes #1451
Two fixes to the TypeScript a scaffolded app actually receives. They were found together, and the second one is what the first one's investigation turned up on the way.
1. The core and server exports an app resolves as
anyhtmlwas typedanyin every scaffolded app, and so werecss,TemplateResult,Suspense,repeat,connectWS,richFetchandescapeText/escapeAttr.packages/core/index.d.tsre-exported those seven modules from their JSDoc.jsimplementation with no.d.tssibling, and an app hasallowJsoff, so each one degraded toany(TS7016), silenced by the scaffold'sskipLibCheck: true. It spread past the direct imports, becausesrc/component.d.ts,src/routes.d.tsandsrc/directives.d.tsreach for the same untyped modules, so a component'srender()return, itsstatic styles, a page's return type andrepeatfrom@webjsdev/core/directiveswere all unchecked.This adds the seven missing overlays. In a real generated app,
render()goes fromanytoTemplateResultandhtmlfrom nothing to(strings: TemplateStringsArray | string[], ...values: unknown[]) => TemplateResult.@webjsdev/servercarried the same class of break in one place.RequestHandler.handlewas typedHandleon the strength of theexport * from './src/testing'above it, butexport *re-exports a name without creating a local binding, so it wasTS2304: Cannot find name 'Handle'andhandlewas an error type. It is imported explicitly now. Fixing it exposed a genuinely too-narrow signature in the gallery's rate-limit test, which had restated the handler type as(req: Request) => Promise<Response>whereHandleis... => Promise<Response> | Response; it derives the type now, per the derive-the-type rule.Also drops the explicit
.d.tsextension from the two valueexport *specifiers (TS2846).Why the existing guards missed it
test/types/dts-export-coverage.test.mjs(#388) andtest/types/dts-no-phantom-exports.test.mjs(#1031) check export EXISTENCE in both directions, and both run tsc with--allowJs. That flag reads the JSDoc out of the.js, which is exactly what an app never gets, so a name that resolves toanyin every app resolved to a real type in both guards. They also assert a name is declared, never that it carries a type.test/types/dts-no-any-exports.test.mjsinverts both flags (allowJsoff,skipLibCheckoff) and grades the overlays the way an app resolves them.2. The scaffold declared a TypeScript it cannot use
The generated
package.jsondeclared"typescript": "^5.6.0"while thetsconfig.jsonthe same generator writes setserasableSyntaxOnly, which landed in 5.8. Every version in the lower half of that range refuses the config outright:exit 2, nothing else checked. Confirmed on 5.6.3 and 5.7.3, both inside the declared range. It stayed hidden because npm resolves a caret to the newest match, so a fresh scaffold picked up 5.9 and worked; it bites a pinned install, an older lockfile, or an editor whose own compiler is older.
The range moves to
^6.0.3, the majorgallery,websiteandexamples/blogalready use, so an app and the framework that generated it type-check under one compiler.test/scaffolds/scaffold-typescript-floor.test.jsties the two files together: it maps every compiler option the generator emits to the release that introduced it and asserts the range's lowest satisfying version clears the highest of those floors. An option missing from the table fails the test rather than being skipped, so adding one has to record its floor.Test plan
node --test test/types/dts-no-any-exports.test.mjs(4/4)node --test 'test/scaffolds/*.test.js'(68/68).d.tsreds the no-any guard; restoring^5.6.0reds the floor guard; adding an unclassified compiler option reds it tootest/bun/listener,test/bun/listener-overhead, and threedifferential-elisionassertions), all runtime tests that pass in the primary checkout and in CIwebjs checkclean, andtsc --noEmitclean under TypeScript 6.0.3, the new floor.htmlandTemplateResultresolve to real types in the generated appwebsite:tsc --noEmitandwebjs checkboth clean after the docs editDoc surfaces
website/app/docs/backend-only/page.ts: its sample api-template manifest showed"typescript": "^5.7.0", itself below the floor, so it demonstrated a config that cannot be read. Corrected to match the generator.gallery/test/rate-limit/rate-limit.test.tsderives its helper param fromHandle.Note on scope
#1451 tracks fix 1 only. Fix 2 has no issue of its own; it was found during the same investigation and folded in here at the owner's request rather than filed separately.