ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): honour the hooks a builder was constructed with

#1074
Merged glennmichael123 wants to merge fix/builder-hooks into main
2 files +176 -19
10 days ago

Closes #1071.

createQueryBuilder({ schema, meta, hooks }) appears in the README and a dozen doc pages, and did not exist — hooks was not a member of the builder's internal state, so the call was a TS2353 excess-property error.

The compile error was the harmless half. as any silences it, and then the hooks never fire: every hook site read the process-wide config.hooks and nothing looked at builder state. A builder that appeared configured ignored every hook it was handed, with no error at any point. A silent no-op is worse than the compile error standing in front of it.

The fix

hooks joins the builder's state, and the 16 read sites go through an activeHooks() resolver instead of reaching for config.hooks directly — the same rule _sql follows after #1065, for the same reason: a builder can be handed its own, and ignoring that makes it silently do nothing.

Two decisions worth calling out, both behaviour rather than plumbing:

  • Hooks merge over the global ones per key. An app-wide logger or tracer shouldn't go quiet because one builder wanted a beforeCreate. A builder hook of the same name still takes precedence.
  • They resolve per call, not once at construction. export const db = createQueryBuilder(...) at module scope followed by setConfig({ hooks }) at boot is the ordinary shape of an app; capturing at construction would silently miss every hook in it.

transaction, savepoint and reserve each build a fresh builder around a different connection — all three now carry the parent's hooks, so hooks don't evaporate on entering a transaction.

Tests

Six cases: the previously-impossible form fires (no cast), a builder without hooks still gets the global ones, per-key merge, same-name override, transaction inheritance, and hooks configured after construction.

Verified they discriminate: against the unfixed client.ts, 4 of the 6 fail.

Typecheck 0, lint 0 errors, suite 1705 pass / 4 skip / 0 fail.

One note for review: I hit an infinite recursion partway through — the mechanical rewrite of config.hooks → activeHooks() also rewrote the reference inside activeHooks itself, which showed up as RangeError: Maximum call stack size exceeded on the first query. Fixed and covered by the tests, but worth a glance at that function.

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.