fix(low-router): harden resolver and createUrl

packages/low-router/src/LowRouter.ts

@@ -1,5 +1,5 @@
 import debug from "@wbe/debug"
-import { PathnameOrObject, Resolve, Route, RouteContext, RouteParams, RouterOptions } from "./types"
+import { PathnameOrObject, QueryParams, Resolve, Route, RouteContext, RouteParams, RouterOptions } from "./types"
 import { createMatcher, Matcher } from "./utils/createMatcher"
 import { compilePath as defaultCompilePath, CompilePath } from "./utils/compilePath"
 import { normalizePath } from "./utils/normalizePath"
@@ -35,10 +35,10 @@ export class LowRouter {
    */
   public async resolve(pathnameOrObject: PathnameOrObject): Promise<Resolve> {
     const obj = this.#resolver(pathnameOrObject)
-    if (typeof obj.context.route?.action === "function") {
+    if (typeof obj.context?.route?.action === "function") {
       obj.response = await obj.context.route.action(obj.context)
     }
-    this.options.onResolve?.(obj)
+    if (obj.context) this.options.onResolve?.(obj)
     return Promise.resolve(obj)
   }
 
@@ -51,7 +51,7 @@ export class LowRouter {
     if (typeof obj.context?.route?.action === "function") {
       obj.response = obj.context.route.action(obj.context)
     }
-    this.options.onResolve?.(obj)
+    if (obj.context) this.options.onResolve?.(obj)
     return obj
   }
 
@@ -62,15 +62,15 @@ export class LowRouter {
    */
   #resolver(pathnameOrObject: PathnameOrObject) {
     // match route
-    const routeContext = this.matchRoute(
+    const pathname =
       typeof pathnameOrObject === "string"
         ? pathnameOrObject
-        : this.createUrl({ name: pathnameOrObject?.name, params: pathnameOrObject?.params })
-    )
+        : this.createUrl(pathnameOrObject)
+    const routeContext = pathname ? this.matchRoute(pathname) : undefined
     // error
     if (!routeContext) {
-      this.#log(`No matching route found with pathname ${pathnameOrObject}`, this.routes)
-      this.options.onError?.()
+      this.#log(`No matching route found with pathname ${pathname}`, this.routes)
+      this.options.onError?.({ pathname, input: pathnameOrObject })
       return { response: undefined, context: undefined }
     }
     // save current context
@@ -81,7 +81,14 @@ export class LowRouter {
   }
 
   public dispose(): void {
-    this.currentContext = null
+    // walk the parent chain to break references and help GC
+    let ctx: RouteContext | undefined | null = this.currentContext
+    while (ctx) {
+      const parent = ctx.parent
+      ctx.parent = null
+      ctx = parent
+    }
+    this.currentContext = undefined
     this.options.onDispose?.()
   }
 
@@ -102,7 +109,9 @@ export class LowRouter {
         let relativePathname: string | undefined
         try {
           relativePathname = this.compilePath(route.path)(params)
-        } catch {}
+        } catch (e) {
+          this.#log("compilePath error", route.path, e)
+        }
 
         const ctx = {
           pathname,
@@ -132,9 +141,20 @@ export class LowRouter {
    * ex:
    *  createUrl({ name: "home" }) => "/"
    *  createUrl({ name: "user", params: { id: "1" } }) => "/user/1"
+   *  createUrl({ name: "user", params: { id: "1" }, query: { tab: "x" }, hash: "top" }) => "/user/1?tab=x#top"
    */
-  public createUrl({ name, params = {} }: { name: string; params?: RouteParams }): string {
-    const next = (name, params, routes, curBase): string => {
+  public createUrl({
+    name,
+    params = {},
+    query,
+    hash,
+  }: {
+    name: string
+    params?: RouteParams
+    query?: QueryParams
+    hash?: string
+  }): string | undefined {
+    const next = (name, params, routes, curBase): string | undefined => {
       for (let route of routes) {
         const compiledPath = normalizePath(this.compilePath(curBase + route.path)(params))
         if (route.name === name) {
@@ -145,7 +165,23 @@ export class LowRouter {
         }
       }
     }
-    return next(name, params, this.routes, this.options.base)
+    const pathname = next(name, params, this.routes, this.options.base)
+    if (!pathname) return undefined
+
+    // append query string
+    let qs = ""
+    if (query) {
+      const sp = new URLSearchParams()
+      for (const k in query) {
+        const v = query[k]
+        if (v != null) sp.append(k, String(v))
+      }
+      const s = sp.toString()
+      if (s) qs = `?${s}`
+    }
+    // append hash
+    const h = hash ? (hash[0] === "#" ? hash : `#${hash}`) : ""
+    return pathname + qs + h
   }
 
   #log(...rest: any[]): void {

packages/low-router/src/types.ts

@@ -5,7 +5,9 @@ export type RouteParams = { [paramName: string]: string }
 export type QueryParams = { [paramName: string]: string }
 export type Hash = string
 export type RouteProps = Record<string, any>
-export type PathnameOrObject = string | { name: string; params: RouteParams }
+export type PathnameOrObject =
+  | string
+  | { name: string; params?: RouteParams; query?: QueryParams; hash?: string }
 
 export interface RouteContext {
   pathname: string
@@ -20,7 +22,7 @@ export interface RouteContext {
 
 export interface Resolve {
   response: any
-  context: RouteContext
+  context: RouteContext | undefined
 }
 
 export interface Route {
@@ -36,7 +38,7 @@ export interface RouterOptions {
   onInit: () => void
   onResolve: ({ response, context }: Resolve) => void
   onDispose: () => void
-  onError: () => void
+  onError: (info: { pathname?: string; input: PathnameOrObject }) => void
   matcher: Matcher
   compilePath: CompilePath
   id?: number | string

packages/low-router/tests/createUrl.test.ts

@@ -99,4 +99,32 @@ describe.concurrent("createUrl", () => {
     )
     expect(router.createUrl({ name: "f", params: { lang: "en" } })).toBe("/en/test/f")
   })
+
+  it("should append query and hash", () => {
+    const routes = [
+      { path: "/", name: "home" },
+      { path: "/user/:id", name: "user" },
+    ]
+    const router = new LowRouter(routes)
+
+    expect(router.createUrl({ name: "user", params: { id: "1" }, query: { tab: "profile" } })).toBe(
+      "/user/1?tab=profile"
+    )
+    expect(router.createUrl({ name: "user", params: { id: "1" }, hash: "top" })).toBe("/user/1#top")
+    expect(
+      router.createUrl({
+        name: "user",
+        params: { id: "1" },
+        query: { tab: "x", y: "z", },
+        hash: "#anchor",
+      })
+    ).toBe("/user/1?tab=x&y=z#anchor")
+    // undefined/null values are skipped
+    expect(
+      router.createUrl({
+        name: "home",
+        query: { a: "1", b: undefined, c: null },
+      })
+    ).toBe("/?a=1")
+  })
 })

packages/low-router/tests/resolve.test.ts

@@ -97,6 +97,51 @@ describe.concurrent("resolve", () => {
     })
   })
 
+  it("should not throw on unmatched pathname", async () => {
+    const router = new LowRouter([{ path: "/foo", action: () => "foo" }])
+    const { response, context } = await router.resolve("/nope")
+    expect(response).toBeUndefined()
+    expect(context).toBeUndefined()
+  })
+
+  it("should call onError and skip onResolve on 404", async () => {
+    const onResolve = vi.fn()
+    const onError = vi.fn()
+    const router = new LowRouter([{ path: "/foo" }], { onResolve, onError })
+    await router.resolve("/nope")
+    expect(onError).toHaveBeenCalledTimes(1)
+    expect(onError).toHaveBeenCalledWith({ pathname: "/nope", input: "/nope" })
+    expect(onResolve).not.toHaveBeenCalled()
+  })
+
+  it("should pass input object to onError when name is unknown", async () => {
+    const onError = vi.fn()
+    const router = new LowRouter([{ path: "/foo", name: "foo" }], { onError })
+    const input = { name: "missing", params: {} }
+    await router.resolve(input)
+    expect(onError).toHaveBeenCalledWith({ pathname: undefined, input })
+  })
+
+  it("should resolve by { name, params }", async () => {
+    const routes = [{ path: "/u/:id", name: "user", action: (ctx: any) => ctx.params.id }]
+    const router = new LowRouter(routes)
+    const { response } = await router.resolve({ name: "user", params: { id: "42" } })
+    expect(response).toBe("42")
+  })
+
+  it("should break parent chain and call onDispose", () => {
+    const onDispose = vi.fn()
+    const router = new LowRouter(
+      [{ path: "/", children: [{ path: "/z", children: [{ path: "/c" }] }] }],
+      { onDispose }
+    )
+    router.resolveSync("/z/c")
+    expect(router.currentContext).toBeDefined()
+    router.dispose()
+    expect(router.currentContext).toBeUndefined()
+    expect(onDispose).toHaveBeenCalledTimes(1)
+  })
+
   it("should resolve child route", () => {
     return new Promise(async (resolve: any) => {
       const routes = [

packages/low-router/tests/resolveSync.test.ts

@@ -85,6 +85,13 @@ describe.concurrent("resolveSync", () => {
       expect(response).toBe("action response!")
   })
 
+  it("should not throw on unmatched pathname", () => {
+    const router = new LowRouter([{ path: "/foo", action: () => "foo" }])
+    const { response, context } = router.resolveSync("/nope")
+    expect(response).toBeUndefined()
+    expect(context).toBeUndefined()
+  })
+
   it("should resolve child route", () => {
       const routes = [
         {