ReviewOS

stacks/stx

useSearchParams: declaration promises delete/has (absent at runtime) and omits setAll/data (present)

#1806
Closed glennmichael123 opened this 11 days ago ·
11 days ago

Verified against @stacksjs/stx at 0.2.153 (local build of main, HEAD 0dcb6fbd0e).

TL;DR

The ambient declaration for useSearchParams() is wrong in both directions: it promises two methods that do not exist at runtime, and omits two that do.

MemberDeclared (stx.d.ts:111-116)Runtime (src/signals.ts:1085)
get✅✅
set✅✅
delete✅❌ absent
has✅❌ absent
setAll❌ not declared✅ present
data❌ not declared✅ present (the backing signal)

Declared

packages/stx/stx.d.ts:111 (hand-written; copied verbatim to dist/stx.d.ts):

declare function useSearchParams(): {
  get: (key: string) => string | null
  set: (key: string, value: string) => void
  delete: (key: string) => void
  has: (key: string) => boolean
}

Implemented

packages/stx/src/signals.ts:1085:

return {
  data: params,
  get: function(key) { return params()[key]; },
  set: function(key, value) { … },
  setAll: function(obj) { … }
};

Why it matters

useSearchParams is in STX_AUTO_IMPORTS (src/client-script.ts:316), so a <script client> block gets it as an ambient global and the declaration is the only contract an author sees.

const params = useSearchParams()
params.delete('q')   // type-checks; TypeError at runtime: params.delete is not a function
params.has('q')      // type-checks; TypeError at runtime
params.setAll({...}) // works at runtime; TS2339 "Property 'setAll' does not exist"

The two failure directions are both bad, but the first is worse: the compiler actively endorses a call that throws in the browser.

Suggested fix

Either add delete / has to the runtime object, or correct the declaration to match what is returned. If the declaration is the thing that changes:

declare function useSearchParams(): {
  data: StxSignal<Record<string, string>>
  get: (key: string) => string | undefined
  set: (key: string, value: string) => void
  setAll: (values: Record<string, string>) => void
}

Note get returns params()[key], i.e. undefined for a missing key — not null as declared.

Same class of defect as #1707 / #1796 / #1797 — a shipped declaration disagreeing with the implementation it describes — but a distinct symbol and a distinct fix. Found alongside two others I am filing separately (navigate, useCookie).

11 days ago

Fixed in e22f5810ca. Both directions, plus two things the report didn't cover.

delete and has are implemented rather than dropped, and the declaration now matches what's returned (data, setAll, and get returning undefined — which the runtime and docs always said).

Beyond the report:

  1. get leaked Object.prototype. The backing object comes from Object.fromEntries, so an unguarded params()[key] returned a function for toString, constructor, valueOf, hasOwnProperty — neither a string nor undefined, matching no declared return type. Reads are own-property now.

  2. There was a third shape. composables/use-router.ts exported a useSearchParams that was a server stub in every environment, browser included — every write silently did nothing. Its sibling useRoute delegates to the runtime, so this was an oversight rather than a design, and it matters because a functions/ composable — the layout the docs prescribe — imports the module path, not the ambient global. It now delegates when the runtime is present and has a real implementation when it isn't.

On your open question about null vs undefined: went with undefined. Zero behaviour change, the in-repo callers are all indifferent (|| '/', if (x), === '1'), and docs/api/composables-ref.md already documented it.

There's now a parity suite running the same 9 assertions against all three entry points (runtime, module-delegating, module-standalone) — 14 of 28 fail with the stub restored.

Two pre-existing issues I found but deliberately left alone, happy to file if useful: every useSearchParams() registers two permanent window listeners with no cleanup, and sibling handles go stale after a set() until the next popstate (pushState doesn't fire popstate, and only the writing closure re-syncs).

Sign in to comment on this issue.