Add type-safe routing with RoutePaths and typed state#23
Merged
Conversation
- Add RawRoutes type constraint for route shape inference
- Add RoutePaths<R> utility type that walks route trees and emits valid path literals
- Excludes guard ('?') keys
- Handles wildcard ('*') segments as ${string}
- Transparent virtual ('') routes
- Add S = any state type parameter to:
- NavMeta<S> and StrictNavMeta<S>
- NavOpts<S>
- Resolve<T, S>
- Routes<T, S>
- Resolved<T, S>
- Router<T, S, R>
- RouterConf<T, S, R>
- OnResolveListener<T, S>
- Add R extends RawRoutes type parameter to Router and RouterConf for routes shape inference
- Update createRouter to infer R from the passed routes via R & Routes<T, S> intersection
- Add typed go() overload accepting RoutePaths<R> for IDE autocomplete
- All changes are backward compatible with defaults
- Add typecheck script to workspace packages
Examples:
const router = createRouter({
routes: { foo: () => 'foo', bar: { baz: () => 'baz' } }
});
router.go('/foo'); // ✓ autocomplete
router.go('/bar/baz'); // ✓ autocomplete
router.go('/unknown'); // type error ✓
interface AppState { userId?: string }
const router = createRouter<string, AppState>({
routes: { foo: ({ state }) => `hello ${state?.userId}` }
});
await router.go('/foo', { state: { userId: '123' } }); // ✓
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a generic navigation-state parameter S across nav/route/router types, introduces RawRoutes and RoutePaths type utilities, adds per-package typecheck scripts and tsconfig.check.json files, and updates tests to use the new generics and stricter typings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Create tsconfig.check.json in each package to include **/*.ts files for full typecheck coverage - Update typecheck script to use tsconfig.check.json instead of tsconfig.json - Add depth limit to RoutePaths utility type to prevent infinite type recursion - Fix type annotations in test files: - Explicitly type NavOpts<number> in route-resolver.spec.ts - Add explicit 'next' parameter types in router.spec.ts - Use 'any' types in render-routes-directive.spec.ts to avoid deep recursion - Add typecheck script to both workspace packages All 40 tests pass + complete typecheck coverage of test files
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/esroute/src/router.ts`:
- Around line 29-32: The go() overload that currently unions RoutePaths<R> with
PathOrHref widens typed paths back to plain string; update the overloads so that
RoutePaths<R> is not unioned with PathOrHref — keep the free-form string/array
escape hatch only on the overload that accepts a { href } / PathOrHref variant
and keep the strongly-typed overload strictly as RoutePaths<R> (or number /
StrictNavMeta<S> / (prev => NavMeta<S>)) so router.go("/not-a-route") no longer
type-checks; apply the same change to the other overloaded method signatures
referenced (the similar overload groups around the other declarations noted in
the review) by separating the typed RoutePaths<R> overload from the PathOrHref/{
href } overload.
In `@packages/esroute/src/routes.ts`:
- Around line 19-22: The index-route branch in RoutePaths currently emits a
trailing-slash string `${Prefix}/`, which mismatches runtime serialization that
strips empty segments; change the branch so an index key (`K extends ""`) yields
the canonical path without a trailing slash: return Prefix (but when Prefix is
the empty string emit "/" — i.e. use conditional logic like `Prefix extends "" ?
"/" : Prefix`) instead of `${Prefix}/` so paths like `/users` type-check
correctly; update the RoutePaths/RawRoutes branch that references `K`/`Prefix`
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20b3afe6-9b46-437f-8f4f-42385e6a7474
📒 Files selected for processing (6)
packages/esroute-lit/package.jsonpackages/esroute/package.jsonpackages/esroute/src/nav-opts.tspackages/esroute/src/route-resolver.tspackages/esroute/src/router.tspackages/esroute/src/routes.ts
added 3 commits
March 31, 2026 19:51
- Refactor router.spec.ts to use properly typed router instance - Extract routes to a separate const and type it with Routes<string> - Use ReturnType<typeof createRouter<string, any, typeof routes>> to get properly inferred type - Add 'type safety' test suite to demonstrate: - Valid typed paths work correctly - NavOpts objects are type-safe - String array paths still work - Router type inference from routes shape works - Demonstrates that IDE autocomplete for router.go() now shows: "/" | "/foo" | "/bar" | "/fail" - All 44 tests pass + complete typecheck coverage
- Remove PathOrHref (string | string[]) from go() signature in favour of
explicit string[] for array paths and RoutePaths<R> for string paths
- Invalid paths like "/fo" are now type errors; only defined routes compile
- Fix router.spec.ts typo: go("/fo") → go("/foo")
- Fix router.spec.ts: go("/foo?a=b") → go("/foo", { search: { a: "b" } })
- Fix router.spec.ts: go("/baz") → go(["baz"]) for intentionally non-existent route
- Cast dynamic anchor href to RoutePaths<R> in link click handler
IsMaxDepth matched every depth level 1-10 instead of only the maximum, causing the first nested route level to immediately widen to string. Changed to only match exactly 10 x's so nesting works up to 10 levels.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/esroute/src/router.ts (1)
29-32: Route handlers still bypass the new path typing.Only
Router.go()is narrowed toRoutePaths<R>. Resolver-side redirects still come fromNavOpts.go(path: PathOrHref)inpackages/esroute/src/nav-opts.ts, wherePathOrHref = string | string[]— untyped and broader thanRoutePaths<R>. This meansnotFoundand route handlers can still emit arbitrary strings, breaking end-to-end path narrowing. To enforce typed routing throughout,Rneeds to flow intoNavOptssogo()can narrow its path parameter toRoutePaths<R>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/router.ts` around lines 29 - 32, The resolver-side redirect API still accepts an untyped PathOrHref (string | string[]) via NavOpts.go, so route handlers can emit arbitrary strings; make NavOpts generic over R and change NavOpts.go(path: ...) to accept RoutePaths<R> (or RoutePaths<R> | number when supporting numeric history navigation) instead of PathOrHref, and update the PathOrHref alias/removals and all NavOpts instantiations/creations so the generic R flows from Router (and any factory functions that construct NavOpts) into NavOpts to enforce end-to-end path narrowing for RoutePaths<R>.packages/esroute/src/router.spec.ts (1)
8-17: Add type-only assertions for the type-safety suite with a concrete state type and edge cases.The test suite only validates that valid flat routes compile and uses
S = anyinstead of a concrete state type. Add assertions to catch regressions in:
- Typed
stateparameter (replaceanywith a concrete type)- Invalid path literals that should fail type checking
- Edge cases:
"*","?", and nested""routesUse
@ts-expect-errororexpectTypeOfto create type-only assertions that verify both valid and invalid paths at compile time.Also applies to: 167-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/router.spec.ts` around lines 8 - 17, The test currently uses a loose state type and only checks flat routes; update the type-safety suite by replacing the generic state parameter S = any in the createRouter instantiation with a concrete state type (e.g., { userId: string }) and add type-only assertions around the routes object and createRouter result: use expectTypeOf to assert valid paths like "" and "foo" resolve to string, add `@ts-expect-error` comments for invalid path literals (e.g., "invalid", nested "" like "nested/"), and add checks for edge cases "*" and "?" to ensure they either accept or reject as expected; target the test helpers around the routes constant and the createRouter<string, S, typeof routes> usage (and duplicate the same assertions in the later suite region) so compile-time failures surface for regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/esroute/src/router.spec.ts`:
- Around line 169-187: The tests call router.init() multiple times without
tearing down global listeners; ensure each initialized router is disposed by
adding a teardown that calls router.dispose() after those tests (e.g., add an
afterEach or explicitly call router.dispose() at the end of the spec blocks that
invoke router.init()) so the global popstate/click listeners installed by
router.init() are removed; reference the router.init() and router.dispose()
calls to locate where to add the cleanup.
---
Nitpick comments:
In `@packages/esroute/src/router.spec.ts`:
- Around line 8-17: The test currently uses a loose state type and only checks
flat routes; update the type-safety suite by replacing the generic state
parameter S = any in the createRouter instantiation with a concrete state type
(e.g., { userId: string }) and add type-only assertions around the routes object
and createRouter result: use expectTypeOf to assert valid paths like "" and
"foo" resolve to string, add `@ts-expect-error` comments for invalid path literals
(e.g., "invalid", nested "" like "nested/"), and add checks for edge cases "*"
and "?" to ensure they either accept or reject as expected; target the test
helpers around the routes constant and the createRouter<string, S, typeof
routes> usage (and duplicate the same assertions in the later suite region) so
compile-time failures surface for regressions.
In `@packages/esroute/src/router.ts`:
- Around line 29-32: The resolver-side redirect API still accepts an untyped
PathOrHref (string | string[]) via NavOpts.go, so route handlers can emit
arbitrary strings; make NavOpts generic over R and change NavOpts.go(path: ...)
to accept RoutePaths<R> (or RoutePaths<R> | number when supporting numeric
history navigation) instead of PathOrHref, and update the PathOrHref
alias/removals and all NavOpts instantiations/creations so the generic R flows
from Router (and any factory functions that construct NavOpts) into NavOpts to
enforce end-to-end path narrowing for RoutePaths<R>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 464ae990-48c1-45b5-adeb-372678297066
📒 Files selected for processing (2)
packages/esroute/src/router.spec.tspackages/esroute/src/router.ts
Comment on lines
+169
to
+187
| router.init(); | ||
| // These should all compile without type errors and accept valid paths | ||
| // The paths "/" | "/foo" | "/bar" | "/fail" are properly typed from the routes | ||
| await router.go("/foo"); | ||
| await router.go("/bar"); | ||
| }); | ||
|
|
||
| it("should work with NavOpts objects", async () => { | ||
| router.init(); | ||
| // Type-safe NavOpts construction with proper typing | ||
| const opts = new NavOpts("/foo"); | ||
| await router.go(opts); | ||
| }); | ||
|
|
||
| it("should work with string array paths", async () => { | ||
| router.init(); | ||
| // String arrays should still work alongside typed string paths | ||
| await router.go(["foo"]); | ||
| await router.go(["bar"]); |
There was a problem hiding this comment.
Dispose the router in these init() cases.
Each init() installs global popstate/click listeners, and these tests never tear them down. Adding three more leaked routers here makes the file more order-dependent and can duplicate later event handling.
🧹 Suggested cleanup
afterEach(() => {
router.dispose();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/esroute/src/router.spec.ts` around lines 169 - 187, The tests call
router.init() multiple times without tearing down global listeners; ensure each
initialized router is disposed by adding a teardown that calls router.dispose()
after those tests (e.g., add an afterEach or explicitly call router.dispose() at
the end of the spec blocks that invoke router.init()) so the global
popstate/click listeners installed by router.init() are removed; reference the
router.init() and router.dispose() calls to locate where to add the cleanup.
When a nested route has "" as a terminal resolver (index route), the path
for that route is the parent segment itself (e.g. "/x"), not with a trailing
slash ("/x/"). Only the root index route produces "/".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/esroute/src/router.spec.ts (3)
22-22: Consider a simpler type alias for readability.The
ReturnType<typeof createRouter<string, any, typeof routes>>expression is verbose. Consider extracting to a type alias if this pattern is reused.💡 Optional simplification
+type TestRouter = ReturnType<typeof createRouter<string, any, typeof routes>>; + describe("Router", () => { let onResolve: Mock; - let router: ReturnType<typeof createRouter<string, any, typeof routes>>; + let router: TestRouter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/router.spec.ts` at line 22, The variable declaration using ReturnType<typeof createRouter<string, any, typeof routes>> is verbose; create a small type alias (e.g., RouterType) capturing ReturnType<typeof createRouter<string, any, typeof routes>> and then declare router as let router: RouterType; update any other occurrences that repeat this pattern; reference the createRouter function and the routes symbol to ensure the alias matches the same generic instantiation.
114-127: Test uses non-existent route "baz".
router.go(["baz"])navigates to a route that doesn't exist in the definedroutesobject. While this may work for testing deferred rendering behavior, using a valid route like["foo"]or["bar"]would be clearer and avoid relying on undefined behavior.💡 Suggested fix
it("should render only once, if render is called with defer function", async () => { await router.render(async () => { - await router.go(["baz"]); + await router.go(["foo"]); await router.go(-1); - await router.go("/foo"); + await router.go("/bar"); }); expect(history.pushState).toHaveBeenCalledTimes(2); expect(history.go).toHaveBeenCalledTimes(1); expect(onResolve).toHaveBeenCalledWith({ - opts: new NavOpts("/foo", { pop: false }), - value: "foo", + opts: new NavOpts("/bar", { pop: false }), + value: "bar", }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/router.spec.ts` around lines 114 - 127, The test calls router.go(["baz"]) but "baz" isn't a defined route; change that call to a valid route (e.g., router.go(["bar"])) so the sequence inside router.render uses existing routes. Update the call to router.go(["bar"]) in the test that invokes router.render(async () => { ... }) so history.pushState, history.go, onResolve and NavOpts("/foo", ...) assertions still exercise deferred rendering behavior with valid routes.
195-206: Type safety assertion is incomplete and doesn't validate at compile time.The manually defined
ValidPathstype is incomplete—it's missing/xand/x/yfrom the routes. More importantly, this test doesn't actually enforce type safety at compile time; it just checks thatrouteris defined at runtime.Consider using a compile-time assertion that derives the paths from the routes:
♻️ Proposed fix to derive paths from routes
it("should infer router types from routes", () => { - // Verify that router is properly typed with inferred routes shape - // The router type is: Router<string, any, typeof routes> - // which means router.go() provides autocomplete for: "/" | "/foo" | "/bar" | "/fail" - - // Demonstrate that valid paths are properly typed - type ValidPaths = "/" | "/foo" | "/bar" | "/fail"; - - // Each valid route is properly typed and provides IDE autocomplete - const router_is_typed: typeof router = router; - expect(router_is_typed).toBeDefined(); + // Compile-time assertion: derive valid paths from routes + type ActualPaths = Parameters<typeof router.go>[0]; + // If RoutePaths is working, this should be: "/" | "/foo" | "/bar" | "/fail" | "/x" | "/x/y" | number | string[] | ... + + // Runtime sanity check + expect(router).toBeDefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/router.spec.ts` around lines 195 - 206, The test currently uses a manually defined ValidPaths and only runtime checks router_is_typed; replace that with a compile-time assertion that derives the path union from the actual routes and ensures it matches the expected set — e.g. create a derived type from the existing routes value (referencing routes and router) and add a TypeScript-only equality/assertion (using a small helper type like AssertEqual or by assigning to a variable with the derived type) so `/x` and `/x/y` are included and the compiler will fail if the inferred paths differ from the expected union instead of relying on a runtime expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/esroute/src/router.spec.ts`:
- Line 5: The import of the symbol "y" from "happy-dom/lib/PropertySymbol" in
router.spec.ts is unused; remove the unused import line (the import statement
that references "y") so the file no longer imports "y" and update any nearby
imports if needed to keep formatting consistent.
---
Nitpick comments:
In `@packages/esroute/src/router.spec.ts`:
- Line 22: The variable declaration using ReturnType<typeof createRouter<string,
any, typeof routes>> is verbose; create a small type alias (e.g., RouterType)
capturing ReturnType<typeof createRouter<string, any, typeof routes>> and then
declare router as let router: RouterType; update any other occurrences that
repeat this pattern; reference the createRouter function and the routes symbol
to ensure the alias matches the same generic instantiation.
- Around line 114-127: The test calls router.go(["baz"]) but "baz" isn't a
defined route; change that call to a valid route (e.g., router.go(["bar"])) so
the sequence inside router.render uses existing routes. Update the call to
router.go(["bar"]) in the test that invokes router.render(async () => { ... })
so history.pushState, history.go, onResolve and NavOpts("/foo", ...) assertions
still exercise deferred rendering behavior with valid routes.
- Around line 195-206: The test currently uses a manually defined ValidPaths and
only runtime checks router_is_typed; replace that with a compile-time assertion
that derives the path union from the actual routes and ensures it matches the
expected set — e.g. create a derived type from the existing routes value
(referencing routes and router) and add a TypeScript-only equality/assertion
(using a small helper type like AssertEqual or by assigning to a variable with
the derived type) so `/x` and `/x/y` are included and the compiler will fail if
the inferred paths differ from the expected union instead of relying on a
runtime expect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95f013ee-adb2-4e3e-9591-44f6d02b96ff
📒 Files selected for processing (2)
packages/esroute/src/router.spec.tspackages/esroute/src/routes.ts
packages/esroute/src/router.spec.ts
Outdated
| import { Router, createRouter } from "./router"; | ||
| import { Routes } from "./routes"; | ||
| import { createRouter } from "./router"; | ||
| import { y } from "happy-dom/lib/PropertySymbol"; |
There was a problem hiding this comment.
Remove unused import.
The y symbol from happy-dom/lib/PropertySymbol is imported but never used in this file. This appears to be an accidental import that should be removed.
🧹 Proposed fix
-import { y } from "happy-dom/lib/PropertySymbol";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { y } from "happy-dom/lib/PropertySymbol"; | |
| // Line 5 is removed - the import statement is deleted |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/esroute/src/router.spec.ts` at line 5, The import of the symbol "y"
from "happy-dom/lib/PropertySymbol" in router.spec.ts is unused; remove the
unused import line (the import statement that references "y") so the file no
longer imports "y" and update any nearby imports if needed to keep formatting
consistent.
added 6 commits
March 31, 2026 22:29
Add type-safe navigation by introducing HandlerFor, StateOf, and NeedsState type utilities that infer required state from route handler signatures. The go() method now requires state when the route handler declares a concrete state type, preventing runtime errors from missing state parameters. Also refactor navigation options handling and update tests to verify state type enforcement works correctly with nested routes.
State is now a non-optional property with definite assignment assertion. Update tests to pass properly typed state and remove optional chaining in assertions.
Change state initialization to always use null as the default value instead of leaving it undefined. This provides a consistent, falsy value for state when not explicitly provided, making the state field definitively typed as `S | null` rather than optional. Update test expectations accordingly.
Change the default generic parameter of NavOpts from `S = any` to `S = null`, and make the state property non-nullable with proper type casting. Update related code to use explicit type parameters for NavOpts and remove unnecessary non-null assertions. Also update NeedsState type guard to check for both null and undefined, fixing type inference for optional state parameters.
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.
Summary
This PR adds comprehensive type safety for route navigation in esroute:
RoutePaths<R>— typos in route paths are caught by TypeScriptNavOpts<S>, and navigation enforces that state is provided with the correct typenull(matching the History API), and is non-nullable when explicitly typedgo()to prevent silent errorsKey Changes
routes.ts: AddedHandlerFor<R, P>,StateOf<F>, andNeedsState<F>type utilities for extracting state types from route handlersrouter.ts: Updatedgo()overloads to require state when route handlers declare it; refactored internal navigation to avoid deep type instantiationnav-opts.ts: Changed default generic param fromanytonull; state is now always assigned (not optional)Breaking Changes
stringis no longer a valid overload forrouter.go()— usestring[]or a valid path string