docs: teach app agents to derive types, never unknown or any - #1200
docs: teach app agents to derive types, never unknown or any#1200vivek7405 wants to merge 9 commits into
Conversation
WebJs sells full-stack type safety, but the agent-facing guide never stated it as a rule and said nothing about unknown, the escape hatch an agent reaches for because it looks safe. State the ladder (schema row types, a named action input, ActionResult, PageProps / LayoutProps / RouteHandlerContext, the generated Route union, prop<T>) and the one place unknown is correct, a payload narrowed on the next line.
An agent copies the example far more reliably than it follows the prose, so every place we taught the anti-pattern is fixed: the layout argument becomes LayoutProps (it was untyped in the generated root layout, which is the first file a new app opens), action signatures get a named input type, the two validate examples narrow a real unknown and return data that satisfies the action's input, and the optimistic temp row drops its as any cast.
CONVENTIONS.md is the short-version essentials list a scaffolded app reads before the skill, so the rule belongs there too. Kept inside the thin-bridge size cap the scaffold integration test enforces.
The blog is the dogfood app its own CONVENTIONS.md now tells agents to learn from, so leaving its three actions on input: unknown contradicted the rule on the same page. Each declares its contract (CreatePostInput, SignupPayload, LoginInput) while the runtime validators stay, since the actions are also reachable from route.ts with a raw JSON body.
Review found the docs-site ActionResult example had lost its runtime guard while the same page documents reaching that action over REST, so an absent title threw instead of returning the 400 the next line promises. The unknown carve-out list was also stated four different ways across the four surfaces, and the three-item version made the scaffold's own template-hole helpers violate it. Also derives the gallery Todo row from the schema (the ladder's top rung, which it was contradicting), types the blog's fourth action, and corrects the temp-id comment, which credited the wrong reason for dropping the cast.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
I ran a fresh pass over this before marking it ready, and it turned up seven things, one of which was a real bug in the diff.
The big one: the ActionResult example on /docs/server-actions lost its runtime guard when I typed the parameter. Two snippets above it, the same page shows that action reached over REST through route(), so a body without a title hit input.title.trim() and threw a sanitized 500 instead of the 400 the very next line promises. My own blog comment in this PR argues for keeping exactly those guards. That is the failure mode this whole PR exists to teach against, so it is now explicit in the prose too: the declared type is a compile-time contract, not a runtime guarantee, and it does not replace validation.
The rest are the rule contradicting itself or the code it governs. I had written the unknown carve-out four different ways across four files, and the shortest version made the scaffold's own template-hole helpers (lede, pageHeading, backLink) violate it. Two more sit outside the diff so I could not anchor them inline. packages/cli/templates/gallery/modules/todo/types.ts hand-writes the Todo row with a comment arguing for it, which is exactly the Not-this in the ladder's top two rungs, and its justification reasons about a VALUE import when #805 exempts a type-only one. And examples/blog/modules/comments/actions/create-comment.server.ts was the app's fourth action, still on a unknown-member union with three (input as any)?. casts plus formatComment(c: any), under the CONVENTIONS.md this PR just amended.
Everything is fixed. The derived-Todo change is the one worth a second look, since it puts import type { todos } from '#db/schema.server.ts' in a module a browser-shipped component imports. I checked it stays off the wire: the generated app's <todo-app> boot imports only its RPC stubs and browser-safe helpers, types.ts is not even in the servable set, and no-server-import-in-browser-module is quiet.
Deferred, not fixed: .agents/skills/webjs/references/typescript.md uses npx webjsdev types in three pre-existing places. The webjsdev wrapper's bin is named webjs and the scaffold pins @webjsdev/cli, so that form fetches an unscoped wrapper from the registry instead of running the installed CLI. I corrected the two lines this PR added and left the pre-existing ones alone.
Round two found the comments formatter had swapped one hand-written row for another when the schema already exports the derived type, which is the anti-pattern the ladder's top rung names; the sibling post formatter had the same shape. Both now derive from the schema. Also rejects a missing body in the docs-site guard (the prose claimed the check made the wire safe while an absent body silently became an empty string), restores npx webjsdev as the documented single-bin form, and squares the docs-site and scaffold-conventions carve-outs with the other four.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, this time scoped to the fix commit. Six more, and one of them says I got a call wrong in round one.
The reversal first: npx webjsdev types was RIGHT and I should not have changed it. packages/cli/lib/create.js L1578 spells out why: the bare webjs npm name is owned by an unrelated package, so npx webjs <cmd> can fetch that one, and webjsdev is our unscoped alias whose single bin npx resolves to webjs. Reverted both lines. Bare webjs types only works inside an npm script, and a generated app has no types script.
The real find: fixing formatComment(c: any) I swapped one hand-written row for another, when db/schema.server.ts already exports Comment = typeof comments.$inferSelect. That is precisely the Not-this in the ladder's top rung, in the commit that was supposed to stop shipping it, and it had already drifted (my CommentRow omitted authorId, which every real row carries). The sibling formatPost had the identical shape. Both now derive from the schema and take the author as Pick<User, 'name' | 'email'>.
Also: the guard I restored on the docs-site snippet coerced a missing body to an empty string and never rejected it, while the prose two paragraphs down claims the check is what makes the wire safe. It now returns the 400, matching the real blog action it mirrors. The optimistic snippet's comment asserted the row type is schema-derived, under which the literal it annotates would not compile (a derived Todo requires the notNull createdAt), so both copies now include createdAt: new Date(), which is what the real gallery component does. And two shipped surfaces still carried their own version of the unknown carve-out: the docs-site page folded the template-hole case into the narrowed-on-the-next-line list, and the scaffold CONVENTIONS.md had no carve-out at all, which made the gallery's own lede / pageHeading / backLink violate it.
The CONVENTIONS.md bullet is trimmed to keep the generated file under the thin-bridge size cap test/scaffolds/scaffold-integration.test.js enforces, with headroom for a longer app name than the fixture's.
The reference the other surfaces point at as the arbiter still filed the html-hole case under narrowed-on-the-next-line, so it is now split into two explicit cases. The optimistic reconcile line in AGENTS.md also diverged from the skill reference and from core's own JSDoc, and was wrong about runtime behaviour: the optimistic row lives in the overlay and auto-releases before the await resumes, so the pending filter is always empty and the expression prepended a row the reducer appended.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third pass, scoped to the previous fix commit. Two substantive, both real.
The canonical reference was still out of step. references/typescript.md is the file every other surface points at as the arbiter, and its unknown section opened with "at the moment before it is narrowed" and then filed the template-hole case under that umbrella as "the third". So the six surfaces I had harmonized all agreed with each other and disagreed with the one that settles it. It is now two explicitly labelled cases, with the template-hole one stated as NOT narrowed.
The other is a behaviour claim I inherited by editing the snippet. The TodoList reconcile line in AGENTS.md read [...this.todos.filter(t => t.pending), result.data, ...this.todos.filter(t => !t.pending)], while the skill reference and core's own JSDoc both use [...this.todos, result.data]. Core registers the release through promise.finally() at .add() time and keeps the optimistic row in the overlay, never in this.todos, so by the time the await resumes that pending filter is always empty and the expression PREPENDS a row the reducer had appended. The row would visibly jump to the top of the list on confirm. Pre-existing on main, but I touched that snippet in both files and left them contradicting each other, so it is fixed here rather than deferred.
Also squared the webjs types / npx webjsdev types spellings inside that one file, which after the round-two revert carried both.
Round four caught the canonical reference excluding constructs this same PR ships (a standalone validator util is a narrowing site but is not one of the three spots the list had closed around), and a genuine collision: case two said an html-hole value is correctly unknown while the ladder calls a layout's children exactly that. The line is authorship. A parameter of your own helper takes unknown; a value the framework already types does not. Also drops a webjs types npm script that no generated app ships.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth pass. Four more, and the sharpest one is a genuine collision I had not seen.
Case two said a value headed for an html template hole is correctly unknown. A layout's children is exactly that, and the ladder two screens up calls { children: unknown } the Not-this, while converting fifteen layouts away from it is the largest change in this PR. The two statements were in the same file. The line turns out to be authorship: a parameter of YOUR OWN helper that forwards into a hole takes unknown (a hole renders a string, a number, a TemplateResult, an array of those, or nothing, so TemplateResult alone is too narrow), whereas a value the framework already types is not that case at all. That is now said explicitly in all four places, and a layout's children joins the not-safe list everywhere.
Second: the previous round had closed the narrowing list to three named spots, and this PR ships unknown narrowing sites outside all three, the standalone validateSignup / validateLogin utils in the blog and validateUser in the backend-only doc page. The arbiter had become stricter than the code it arbitrates. It is open-ended again.
Third: I invented an npm script. No generated app ships a types entry, and packages/cli/lib/create.js writes the scripts block without one. The row points at npx webjsdev types again, noting that npm run dev also emits the file.
Fourth: my reconcile comment fixed the code and then misattributed the reason. this.todos holds only confirmed rows because the optimistic row lives in the overlay and is never written to the prop, not because the release already ran. Release timing governs .value, not this.todos. Restating the overlay-versus-prop confusion in the comment that fixes it is a good way to reintroduce it later.
The round-four scoping landed on five surfaces and missed the two that ship into every generated app, so a scaffolded app carried both the scoped rule and an unscoped one contradicting it. Also corrects the overlay claim (it stores the payload, and update rebuilds the row on each read, so no row is stored) and types docs-shell's children, which the website was widening back to unknown one call after LayoutProps typed it.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fifth pass, and it caught me shipping the fix to five surfaces and missing the two that matter most.
The round-four scoping (a template-hole unknown is about a parameter of YOUR OWN helper, not a value the framework already types) landed in the skill reference, root AGENTS.md, SKILL.md, the docs site, and the blog conventions. It did not land in packages/cli/templates/AGENTS.md or packages/cli/templates/CONVENTIONS.md, which are the two files webjs create writes into every generated app. Since the same generator also copies the fixed skill reference in, a scaffolded app was carrying the scoped rule and the unscoped one that contradicts it, side by side. Both are updated.
The reconcile comment was also still wrong about the mechanism, one round after I corrected it for being wrong about the mechanism. The overlay stores the PAYLOAD, not the row; update rebuilds the row on every .value read. The load-bearing half (never written to this.todos) was right both times, but "the row lives in the overlay" is the same overlay-versus-prop confusion in a new coat.
And the website was widening children straight back to unknown: docs/layout.ts takes LayoutProps, then hands that TemplateResult to docsShell({ children }) whose field was declared unknown. Typed now. asideTop stays unknown in the same interface, correctly, since it is an arbitrary hole value from anywhere.
This is where I am stopping the review loop: five rounds is the budget, and this round found substantive problems, so the loop has NOT converged. The fixes above are unverified by a further round. One finding is deferred rather than fixed, below.
|
Deferred finding: the temp id sits inside the optimistic reducer The round-five review also flagged that the canonical Fixing it means changing the snippet's shape (the payload stops being a bare |
1 participant
Closes #1199
Summary
WebJs sells full-stack type safety as a headline capability, but the guidance an app-building agent actually reads never stated it as a rule, and several of our own canonical examples modelled the opposite. This states the rule and fixes every example that contradicted it.
The rule.
.agents/skills/webjs/references/typescript.md(the canonical reference, copied verbatim into every scaffolded app) now opens its full-stack-type-safety section with the ladder: derive a row type from the schema (typeof todos.$inferSelect), carry it across the.serverboundary withimport type, name the action's input interface and returnActionResult<T>, type routing files withPageProps/LayoutProps/RouteHandlerContext, generate theRouteunion, narrow a reactive property withprop<T>(). It also says whereunknownis still correct and whyanygets no such carve-out. The rule is referenced from the repoAGENTS.md, the skill'sSKILL.mddefault workflow, the scaffold'sAGENTS.mdandCONVENTIONS.md, and the docs site's TypeScript page.unknownis deliberately not banned outright, and pinning down exactly when it is right took most of the review. Two cases, and only the first is about narrowing:route.tsawait req.json(), an action'sexport const validateor a validator it delegates to, acatchbinding. The test is what the next line does.htmltemplate hole. A hole renders a string, a number, aTemplateResult, an array of those, or nothing, solede(content: unknown)is correctly typed andTemplateResultalone would be too narrow. This one is never narrowed, and it is about a value you accept, never one the framework already types: a layout'schildrenalso lands in a hole, butLayoutPropstypes it, so{ children: unknown }is a discarded type rather than this carve-out.Everywhere else it is a missing type, not a safe one:
unknownsurviving into a return type, a component prop, a layout'schildren, or an action signature is the shape to fix.The examples. An agent copies the example far more reliably than it follows the prose, so a contradicted rule is a dead rule. Fixed:
{ children: unknown }becomesLayoutPropseverywhere, including the GENERATED root layout (packages/cli/lib/create.js), the blank-slate layoutgallery:clearwrites, the three gallery layouts, the website's own three layouts, and the layout snippets on five docs pages.LayoutProps.childrenis aTemplateResult, so this is a real type where there was none, in the first file a new app opens.createPost(input: unknown)under a heading about seeing the real function signature. Same for/docs/server-actions.validate = (input: any)examples become a properly narrowed(input: unknown)whose returneddatasatisfiesthe action's input type, which is what carries a real type into the action body.as any(the row type already carries the client-onlypending?: boolean).let browser: any, page: any, serverProcess: anyfor aChildProcessplus two minimal structural types, with a comment on swapping them for the realpuppeteer-coreimports once it is installed.CreatePostInput,SignupPayload,LoginInput) while keeping the runtime validators, since they are also reachable fromroute.tswith a raw JSON body.Test plan
npx tsc --noEmiton the website app: clean (the only errors are the pre-existing#modules/ui/components/*mirror, gitignored and absent in a fresh worktree)webjs checkon the website app: all checks pass/,/docs/typescript,/docs/server-actions,/docs/backend-only,/docs/routing,/docs/ssr,/docs/styling,/docs/getting-started,/uiwebjs createfull-stack: generated app typechecks clean,webjs checkpasses, boots and serves/,/features/routing,/features/forms,/features/auth,/examples/todo.<gallery-nav current="/features/routing">confirms thenew URL(url).pathnamesimplification theLayoutPropsconversion allows.npm run gallery:clearon that app: blank slate still typechecks clean and passeswebjs check, with the minimal layout onLayoutPropswebjs create --template api: typechecks clean,webjs checkpassesnode --test test/scaffolds/*.test.js: 50/50 pass (theCONVENTIONS.mdaddition was trimmed to stay inside the thin-bridge size cap that suite enforces)npx tsc --noEmitadds no new errors,node --testover auth / posts / comments 36/36 passnpm test: 2972/3043 pass. The 70 failures are environmental in this worktree (symlinkednode_modules, no per-app install, dev-server spawn tests timing out). Verified against a baseline worktree atorigin/mainwith the identical setup: 2960/3033 with 72 failures, and the failing set is identical modulo two blog-DB tests that fail on baseline and pass here. CI runs a realnpm ciand is the real signal.Doc surfaces
AGENTS.md: updated (new## Typessection, the reference-table row, the optimistic example).agents/skills/webjs/: updated (SKILL.mdrouting line + workflow step 8,references/typescript.md,data-and-actions.md,optimistic-ui.md,routing-and-pages.md)typescript,server-actions,backend-only,routing,ssr,styling,getting-started)packages/cli/lib/create.js,templates/AGENTS.md,templates/CONVENTIONS.md,templates/gallery/**,templates/scripts/clear-gallery.mjs,templates/test/hello/e2e/hello.test.ts). The per-agent files (CLAUDE.md,GEMINI.md,.cursorrules,.github/copilot-instructions.md) are thin bridges toAGENTS.mdand the skill, so they need no change.examples/blog/CONVENTIONS.mdplus the three actions it governsREADME.md: N/A, its "type safety runs the full stack" claim is unchanged and still accurateinitprimer sources theExecution modelandInvariantsheadings, neither of which movedtest/bun/*.mjsandpackages/server/test/**inline app fixtures still write{ children: unknown }. Left alone on purpose: they are synthetic single-file test apps that do not import@webjsdev/coretypes, and they teach nobody.Review status
Five review rounds ran. The last one still found substantive problems (the round-four scoping had landed on five surfaces and missed the two that ship into every generated app), so the loop did NOT converge; it stopped on its round budget. The round-five fixes are in and verified against the suites below, but they have not themselves been through a round.
One finding is deferred rather than fixed, recorded in a comment on this PR: the canonical
TodoListsnippet callscrypto.randomUUID()inside theoptimistic()reducer, so the pending row gets a fresh id on every read. The scaffold's real component does it correctly by computing atempIdoutside. Fixing it changes the snippet's payload shape across two surfaces plus core's own JSDoc, which is a design call rather than a mechanical fix.Not in scope
No
webjs checkrule. Bothunknownandanyare valid TypeScript that a sensible app could legitimately want in a given spot, so by the conventions-vs-checks split inAGENTS.mdthis is prose guidance, not a correctness gate.