ReviewOS

stacks/stx

window.stx.useEventListener is typed target-first but implemented event-first: the working call is a type error, and the typed call silently binds nothing

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

window.stx.useEventListener is declared with a leading target parameter that the implementation does not have. Calling it the way it actually works is a type error; calling it the way it is typed attaches no listener.

The two disagree

Implementation — packages/stx/src/signals.ts:5566, and this is the function put on the registry at 6284, which becomes window.stx at 6226:

function useEventListener(event, handler, options) {
  var target = (options && options.target) || window;
  if (typeof target === 'string') target = document.querySelector(target);
  if (!target) return;
  ...
}

Event first. The target is optional and travels in options.target.

Declaration — dist/stx.d.ts:274:

declare function useEventListener<K extends keyof WindowEventMap>(
  _target: Window | Document | HTMLElement | string | null,
  _event: K,
  _handler: (event: WindowEventMap[K]) => void,
  _options?: boolean | AddEventListenerOptions,
): StxCleanup

Target first.

Why it is worse than a wrong signature

_target accepts string, so the correct two-argument call type-checks as far as the first parameter — 'keydown' is read as a selector — and then fails on the second, where the handler lands in an _event: K slot:

window.stx?.useEventListener('keydown', (event: KeyboardEvent) => { ... })
// No overload matches this call. (TS2769)

An author who trusts the type and adds the target gets code that runs and silently does nothing, because useEventListener(window, 'keydown', fn) binds event window, handler 'keydown'.

Found in a real app whose keyboard-shortcut block calls the two-argument form. That code is correct and works; it is the checker that objects.

The standalone export is right

dist/browser-composables.d.ts:38 declares the same name correctly:

export declare function useEventListener<K extends keyof WindowEventMap>(event: K, handler: (e: WindowEventMap[K]) => void, options?: AddEventListenerOptions): () => void

So the package ships two declarations of one name with different arities, and the registry got the one that does not match the runtime. Same shape as the reactive split — one name, two conventions, and the call site cannot tell which it has.

Fix

Type the registry entry from the implementation: (event, handler, options?) with the target carried on options.target. Aliasing the registry member to the browser-composables declaration would keep them from drifting again.

10 days ago

Fixed in 968a541. Your diagnosis was exact, including which of the two declarations was the wrong one.

Typed from the implementation now — (event, handler, options?) with the target on a named StxEventListenerOptions. Both directions are pinned, because fixing only the first would have left the silent no-op type-checking:

useEventListener('keydown', (event) => { … })                    // ✅ was TS2769
useEventListener('click', () => {}, { target: '#panel' })        // ✅
useEventListener(window, 'keydown', (e: KeyboardEvent) => {})    // ✅ now TS2769

There is also a test asserting stx.d.ts and browser-composables.ts still agree on which parameter leads, so the split you identified cannot silently reopen.

Why this was hiding, which is worth knowing for the next one

I edited the declaration, re-ran, and the old signature was still in force. packages/stx/stx.d.ts imports @stacksjs/stx, which resolves to dist/index.d.ts, which references dist/stx.d.ts — a second copy of itself. Both files load, and TypeScript merges the two declare function sets into one overload list.

So while dist was stale, the old target-first overloads were still live, and the merged list accepted both spellings. The error text is what gave it away — tsc reported a parameter type of string | Document | HTMLElement | Window | null, which was _target from a declaration I had just deleted.

That has a practical consequence for your original report: dist/stx.d.ts:274 and the source were the same file at different ages. It also means a stale dist can make a correct source declaration appear broken, which is a nasty trap for anyone debugging types in this repo. Rebuilding resolves it; I have not changed the arrangement, but it is worth an issue of its own if you want the double-load removed.

Verification: 5 tests, sabotage-checked by restoring the target-first overload — 3 fail, including the working-call and the silent-no-op cases.

Full suite 11894 pass; the 2 failures on that run are the craft binary resolution pair, which fail only on machines that have craft installed and pass on CI.

Not in v0.2.176 — this lands in the next release.

Sign in to comment on this issue.