fix(route): use {} for static-segment path inference (closes #178)#179
Conversation
InferRoutePatternSegment returned EmptyRecord (= Record<string, never>) for
static route segments. When intersected via '&' in
InferRoutePatternPathSegments, the index signature [k: string]: never
collapsed all dynamic-segment fields to never, breaking
createRouteReference('/channel/[id]').href({ path: { id: 'c-123' } }) with
TS2322.
- packages/fresh/src/application/route/types.ts:114
EmptyRecord -> {} so static segments contribute no index signature when
intersected with dynamic segments.
- packages/fresh/src/application/route/types.ts:120
Flatten the intersection (InferRoutePatternSegment<TSegment> &
InferRoutePatternPathSegments<TRest> & {}) so TypeScript collapses it to
the dynamic-segment object shape.
- packages/fresh/src/application/route/_internal/contract-types.ts:74
Mirror the EmptyRecord -> {} change. (Line 78 already wraps the
intersection in Simplify<>, no change needed.)
- packages/fresh/src/application/route/contract.test.ts
Add direct (non-cast) regression assertions for the six validation
patterns from the issue. The pre-existing test around line 176 used
'as unknown as' casts that masked the bug.
Refs: #177 (original user report), WI-12 (downstream codegen depends on this).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
plan-eval verdict: ❌ REQUEST CHANGES
8b. Additional finding not in the checklist — CI lint gate is REDCI run Three
Notes
Recommendation❌ REQUEST CHANGES The PR implements the issue spec correctly and the regression test is well-designed (verifiably catches the bug when reverted). But the CI Required before re-eval:
|
The earlier patch in 5a12ef9 (closes #178) introduced three \{}\ literals to replace EmptyRecord on InferRoutePatternSegment's static branch and as an intersection flatten in InferRoutePatternPathSegments. CI quality gate (run 28432284595) tripped deno-lint's ban-types rule on those three sites. Extract a local \EmptySegment\ alias in each file and reference it from the type expressions, suppressing the lint rule with a documented \// deno-lint-ignore ban-types\ comment directly above each alias declaration. The alias is not exported (it is a routing implementation detail); EmptyRecord remains the public empty-pattern discriminator used by RouteContractPathOf / RouteContractSearchOf and the contract machinery. Type-soundness preserved: - \deno task lint\ exits 0 - \deno task check\ exits 0 (1808 files, zero errors) - \deno check --unstable-kv packages/fresh/src/application/route ...\ exits 0 across the route module and the CLI downstream consumers (route-templates_test.ts, app-route-seeds.ts) - \deno test --no-check packages/fresh/src/application/route/contract.test.ts\ still reports 10 passed | 0 failed, including the #178 regression test - Reverting the alias (and using EmptyRecord instead) reproduces the original 4 TS2322 errors on the regression test, confirming the test still catches the bug Refs: #178, #177. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
[PHASE: PLAN-EVAL] [VERDICT: APPROVED] Re-eval after CI lint fix on commit
Notes
Recommendation✅ APPROVE — All 10 checks pass with concrete evidence. The implementation correctly threads the needle between Re-eval session: Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com |
🤖 Augment PR SummarySummary: Fixes Fresh route-pattern path inference so static segments no longer collapse dynamic params to Changes:
Technical Notes: Keeps 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| * `never`). | ||
| */ | ||
| // deno-lint-ignore ban-types | ||
| type EmptySegment = {}; |
There was a problem hiding this comment.
type EmptySegment = {} can widen the public route path type so that non-object values may be accepted for path at call sites (since {} is satisfied by many non-nullish primitives). Please double-check that this widening is acceptable for createRouteReference()/href() inputs when a route has no dynamic params.
Severity: medium
Other Locations
packages/fresh/src/application/route/_internal/contract-types.ts:76
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Re-eval of the
EmptySegment = {}widening concern on PR #179 (head282fea54).
Confirmed real, but bounded. I verified Augment's claim independently on the PR head:
staticRoute.href({ path: 42 })— passes type-check on the PRstaticRoute.href({ path: true })— passes type-checkstaticRoute.href({ path: [1, 2, 3] })— passes type-checkstaticRoute.href({ path: "literal" })— passes type-checkstaticRoute.href({ path: null })— rejected (TS2322 againstEmptySegment | undefined)
Same widening accepts primitives, arrays, booleans, functions. On main, all of those are rejected with TS2322 (Type 'number' is not assignable to type 'EmptyRecord'). The widening leaks through createRouteReference → InferRoutePatternPath → RoutePathInput → RouteHrefInput because EmptySegment is {} (= NonNullable<unknown>) instead of Record<PropertyKey, never>.
Builder surface is unaffected. bindRoutePattern + getLinkProps flows through TypedRoutePathInput which uses HasPathParams<TPath> (keyof-based), and on the PR branch TPath resolves to EmptyRecord (not EmptySegment) for empty paths, so the ?: optional branch still narrows to EmptyRecord. Verified: ref.href({ path: 42 }) errors with Type 'number' is not assignable to type 'EmptyRecord' on the builder surface — i.e., the widening is contained to the createRouteReference / pattern-string flow.
Practical risk for #178 / WI-12 scope: low. The widened public surface for static-only patterns accepts junk for path, but:
- Runtime href builders do not dereference
pathwhen the pattern has no dynamic params (they returnroutePatternas-is from$href), so non-objects cannot corrupt output. - The contract-builder surface (
bindRoutePattern/defineRouteContract) is unaffected because it routes throughEmptyRecord, notEmptySegment. - The original 2-line patch +
EmptySegmentis the minimum fix for the fix(route): InferRoutePatternSegment returns EmptyRecord for static segments, breaks multi-segment dynamic paths #178 intersection-collapse bug — switching back toEmptyRecordre-introduces TS2322 errors on/channel/[id],/posts/[[...slug]], etc. (the regression test catches this).
Recommendation: the implementation is correct for the #178 fix; Augment's widening is a real but low-risk side effect of using {} as the empty-segment marker. The cleanest refinement is to swap EmptySegment for a non-widening empty alias that still avoids the index-signature collapse — e.g., type EmptySegment = { readonly __empty?: never }, which keeps intersections flat without widening to NonNullable<unknown>. That would be a follow-up PR; for #179 the current fix is acceptable.
Plan-eval verdict remains APPROVE (see #179 (comment)). The widening does not change the merge-readiness gate, but flagging for a follow-up refinement is warranted.
|
[PHASE: IMPL-EVAL] [VERDICT: APPROVED] [FOLLOW-UP: augment-review noted] Augment flagged a single medium-severity inline concern (review 4599225944, comment 3497777373) on Independent verificationI built and ran a probe on the PR head (
The widening is real for the
Practical risk for #178 / WI-12Low. The widened surface accepts junk for RecommendationKeep plan-eval verdict as APPROVE (see #179 (comment)). Augment's concern does not change the merge-readiness gate. Follow-up refinement (separate PR, optional): swap Replied to the inline thread with the same analysis: #179 (comment) |
…Contract
Planning artifact for the WI-12 implementation. The actual code changes
(restored .withRouteContract({...}) builder method, generator AST scan,
Vite plugin page-module rewriting, Form A/B/C algorithm, tests,
migration, README updates) will follow in subsequent commits on this
branch.
Refs: #181, #179
…ge-module route binding (WI-12) (#183) * docs(wi): add WI-12 - page-module codegen for .withRoute / .withRouteContract Planning artifact for the WI-12 implementation. The actual code changes (restored .withRouteContract({...}) builder method, generator AST scan, Vite plugin page-module rewriting, Form A/B/C algorithm, tests, migration, README updates) will follow in subsequent commits on this branch. Refs: #181, #179 * chore(openhands): record run trace 28474310084-1 * feat(fresh): restore inline .withRouteContract builder method (WI-12) Add `.withRouteContract({ $route?, pathSchema?, searchSchema? })` back to `definePage()` as a first-class, type-state-aware builder method. The inline schema body is bound to the `$route` pattern via `bindRoutePattern(defineRouteContract({...}), $route)`, producing the same routed runtime config that `.withRoute(...)` consumes, so the inline (Form A) and sidecar/default (Form B/C) authoring forms converge on one runtime shape. The `$route` pattern is normally inserted by the NetScript Vite plugin codegen pass from the page module path; a missing `$route` throws a clear build error pointing at the codegen / manual-fill remedy. Type-state promotion mirrors `.withRoute(...)`: path/search are inferred from the inline schemas and `THasConfiguredRoute` flips to `true`. The public `PageBuilder` compat surface gains the matching method signature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(fresh): scan page modules for inline route contracts in generator (WI-12) Add a dependency-free page-module scanner (`manifest-page-module.ts`) that detects `.withRouteContract({...})` (Form A) and `.withRoute(...)` calls using balanced brace/paren matching, and extracts the inline contract's schema body (stripping any pre-filled `$route`). Discovery now classifies every page module as `inline` / `sidecar` / `default` and errors when a module carries both `.withRoute` and `.withRouteContract`. Resolves WI open-question #6: the synthesized inline contract lives in the page module, not in `.generated/routes.ts`. The inline schema body references page-module-scoped imports (`z`, custom param schemas) that are out of scope in `.generated/routes.ts`, so re-emitting it there would not type-check. Form A's typed binding is therefore done in the page module via the restored `.withRouteContract({ $route, ...body })`, and the `routes.<key>` leaf for a Form A page is a schema-free `createRouteReference` (same shape as Form C). Form B continues to bind the imported sidecar contract. Existing manifest behaviour and tests are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(fresh): page-module route-binding codegen + pageModuleRouteBinding option (WI-12) Implement the WI-12 page-module rewriter and wire it into the Vite plugin, covering commits 3-5 of the plan (rewriting, Form C default, conflict resolution) as one coherent pass so each is green-per-commit. - `computePageModuleRewrite` (manifest-page-module.ts): idempotent rewriter. Form A inserts `$route: routePatterns.<key>.$route` as the first field of the inline `.withRouteContract({...})` and ensures the `routePatterns` import. Form B/C insert `.withRoute(routes.<key>.$route)` immediately after `definePage()` and ensure the `routes` import. Re-running produces no diff. - `writeNetScriptPageModuleBindingsSync` (manifest.ts): disk orchestrator that rewrites each discovered page module and collects build warnings. - Conflict resolution: inline + sidecar emits the "inline takes precedence" warning (sidecar left in place); `.withRoute` + `.withRouteContract` in one module is a build error raised during discovery. - Vite plugin: new top-level `pageModuleRouteBinding` option (default `true`). The binding pass runs after the manifest writer on init, buildStart, and the debounced watch handler; warnings surface via `console.warn`. Setting it to `false` skips all page-module rewriting and leaves hand-written bindings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(fresh): cover WI-12 inline contract, AST discovery, rewriting, conflicts Add unit tests for the WI-12 surface: - builder.test.tsx: `.withRouteContract({ pathSchema })` / `({ searchSchema })` type-state promotion + runtime href binding, and the clear error when `$route` is missing. - manifest.test.ts: inline-contract discovery (Form A positive), non-page helper with `.withRouteContract` ignored (negative), and the build error when a page has both `.withRoute` and `.withRouteContract`. - manifest-page-module.test.ts (new): scanner body/`$route` extraction, `.withRoute` vs `.withRouteContract` discrimination, Form A/B/C rewriting, idempotency across forms, and the inline+sidecar precedence warning. - vite.test.ts: `pageModuleRouteBinding` default rewrites page modules (with a second-pass idempotency assertion) and `false` leaves them untouched while still writing the generated routes module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(fresh): document Form A/B/C codegen route binding (WI-12) Update the package README and the web-layer builders guide so the codegen-owned route-binding flow is the primary documented path: - README Usage now leads with Form A (inline `.withRouteContract`), Form B (sidecar), and Form C (default), and reframes the manual `bindRoutePattern` + `.withRoute` example as the under-the-hood / opt-out form. This corrects the long-standing drift where the manual form was documented as primary. - builders.md gains a `.withRouteContract({...})` row in the methods table and a "Codegen-owned route binding" subsection covering the three forms, the inline+sidecar warning, the dual-binding error, and the `pageModuleRouteBinding: false` opt-out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(wi): record WI-12 implementation status and deferred CLI migration Document the green implementation status (commits 1-6, 8), the end-to-end typecheck proof, the `paths?` field omission rationale, and the deliberate deferral of the live CLI scaffold-template migration (commit 7) — the templates bind through the curated `@app/router.ts` with hand-authored route references that do not all map to the generated `routes` tree, and the proving gate is the toolchain-heavy scaffold.runtime E2E lane that cannot run in this environment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(openhands): record run trace 28478153536-1 * chore(openhands): record run trace 28483118420-1 --------- Co-authored-by: OpenHands Bot <openhands@all-hands.dev> Co-authored-by: Eric Chautems <eric.chautems@eisberg.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Closes #178.
InferRoutePatternSegmentreturnedEmptyRecord(=Record<string, never>) for static route segments. When intersected via&inInferRoutePatternPathSegments, the index signature[k: string]: nevercollapsed all dynamic-segment fields tonever, breaking the public API:This is a regression introduced during the netscript-start → netscript port. The prior implementation in
netscript-start/packages/fresh/route/contract.ts:65-74already used{}for static segments and wrapped the intersection inSimplify<>. This PR restores that.Diff
packages/fresh/src/application/route/types.ts/** Infer one typed path segment from a Fresh route pattern segment. */ export type InferRoutePatternSegment<TSegment extends string> = TSegment extends `[[...${infer TParam}]]` ? { [TKey in TParam]?: readonly string[] } : TSegment extends `[...${infer TParam}]` ? { [TKey in TParam]: readonly string[] } : TSegment extends `[${infer TParam}]` ? { [TKey in TParam]: string } - : EmptyRecord; + : {}; /** Infer typed path params from the stripped Fresh route pattern. */ export type InferRoutePatternPathSegments<TPattern extends string> = TPattern extends '' ? EmptyRecord : TPattern extends `${infer TSegment}/${infer TRest}` - ? InferRoutePatternSegment<TSegment> & InferRoutePatternPathSegments<TRest> + ? InferRoutePatternSegment<TSegment> & InferRoutePatternPathSegments<TRest> & {} : InferRoutePatternSegment<TPattern>;packages/fresh/src/application/route/_internal/contract-types.tstype InferRoutePatternSegment<TSegment extends string> = TSegment extends `[[...${infer TParam}]]` ? { [TKey in TParam]?: readonly string[] } : TSegment extends `[...${infer TParam}]` ? { [TKey in TParam]: readonly string[] } : TSegment extends `[${infer TParam}]` ? { [TKey in TParam]: string } - : EmptyRecord; + : {};(Line 78 in the internal file already wraps the intersection in
Simplify<>, so no further change is needed there. Intypes.tswe deliberately use& {}rather than introducing a newSimplifyimport — the trailing empty object literal flattens the intersection identically without expanding the file's dependency surface.EmptyRecorditself is preserved — it remains the discriminator for "no path params" used attypes.ts:162-171and many other places.)Validation cases
All six cases from the issue's validation table are now covered by direct (non-cast) compile-time assertions in
packages/fresh/src/application/route/contract.test.ts:InferRoutePatternPath/channel/[id]{ id: string }/session/[a]/[b]{ a: string; b: string }/[id](single dynamic, no static prefix){ id: string }/static/only{}/posts/[[...slug]]{ slug?: readonly string[] }/dashboard/products/[id]/edit{ id: string }The existing runtime cases in
contract.test.ts:176-235continue to pass.How to verify
From the repo root, run the scoped package check (the wrapper-scoped gate that CI runs):
Type-check the affected surface directly:
Run the route contract tests:
deno test --allow-all --no-check packages/fresh/src/application/route/contract.test.tsExpected:
10 passed | 0 failed, including the newInferRoutePatternPath infers static segments as {} (regression for #178)test.Verify the session-workspace repro from the issue no longer errors. The published JSR repro at
C:\Users\chaut\.copilot\session-state\2004fad4-33a3-4744-9a50-25e4267cc7b4\files\repro\bug-typecheck.tsimports fromjsr:@netscript/fresh@0.0.1-alpha.12/route, so it requires a published fix. A workspace-local equivalent (importing from the patched source) compiles cleanly against this branch:deno check --config deno.json <(echo "import { createRouteReference } from 'packages/fresh/src/application/route/mod.ts'; const c = createRouteReference('/channel/[id]'); type I = NonNullable<Parameters<typeof c.href>[0]>; type P = I extends { path: infer P } ? P : never; const ok: P = { id: 'c-123' };")Expected: no
TS2322error.Regression coverage
I verified that the new direct assertions catch the bug by reverting the patch and re-running
deno check packages/fresh/src/application/route/contract.test.ts— without the patch, four of the six patterns fail withTS2322(/channel/[id],/session/[a]/[b],/posts/[[...slug]],/dashboard/products/[id]/edit). With the patch applied, all ten tests pass anddeno checkis clean.Out of scope
References
neverpath params for multi-segment patterns (EmptyRecord intersection) #177 — original user report (analysis chain)