ReviewOS

also looking at this

stacks/bun-query-builder

fix(types): config options should not require what the library already defaults

#1064
Merged glennmichael123 wants to merge fix/config-options-deep-partial into main
12 files +885 -117
10 days ago

Closes #1062.

The problem

QueryBuilderConfig describes the config after defaults are merged in, so all twelve top-level fields are required. It was also the only config type this package exported, so it doubled as the type apps annotate their own query-builder.config.ts with — and in that role every required field is a bug. Adding migrationDir: string turned tsc --noEmit red in stacksjs/stacks, and the downstream fix was hard-coding 'database/migrations', a value this package already defaults in three places. snapshotDir did the same a release earlier.

The fix

Export the input shape separately:

export type QueryBuilderOptions = DeepPartial<QueryBuilderConfig>
export function defineConfig(userConfig: QueryBuilderOptions): QueryBuilderOptions

QueryBuilderConfig stays the resolved type internal code relies on, so config.pagination.defaultPerPage still needs no guard. defineConfig() means a config file needs no annotation decision at all.

// query-builder.config.ts — nothing restated
import { defineConfig } from 'bun-query-builder'

export default defineConfig({
  dialect: 'postgres',
  database: { database: 'my_app' },
})

DeepPartial is not the one-liner in the issue

The issue proposes { [K in keyof T]?: T[K] extends object ? DeepPartial<T[K]> : T[K] }. I verified that version does not fix the reported bug:

  • For an optional member, T[K] is X | undefined. That is not a naked type parameter, so it does not distribute, fails extends object, and passes through still-required — { softDeletes: { enabled: true } } keeps demanding column.
  • A homomorphic map rewrites sqlStates: string[] as (string | undefined)[], which no longer assigns back to string[].
  • keyof (() => void) is never, so hooks collapse to a non-callable empty object loose enough to accept 42 — a call signature is not a property.

Hence the ordered guards: functions → arrays → index signatures. Array leaves come back readonly so the idiomatic export default { … } as const config file assigns.

The type change could not ship alone

applyConfig was Object.assign plus five hand-written nested spreads, so nine sections — plus database.pool and transactionDefaults.backoff one level down — were replaced wholesale. The over-strict type had been standing in for a merge that did not exist: it forced callers to hand over complete sub-objects. Widening it without a real merge would have traded a compile error for transactionDefaults.backoff being undefined at runtime, which is strictly worse.

The merge is now recursive, with semantics pinned by tests: arrays and functions replace, an explicit undefined assigns while an absent key does not, the top-level config identity is preserved (Bun's bundler splits reassigned bindings), and nested nodes are replaced rather than mutated (seven test files capture a nested node and hand it back to restore it). db.configure() routes through it instead of bypassing it.

Two regressions caught in review and removed

An adversarial review raised 19 findings; 13 were refuted, 6 confirmed. Two were serious:

  • The config-file veto stays per SECTION. I had made it per-leaf, which reads as more precise and is wrong: database.url and the discrete host/port/username/password are mutually exclusive spellings of one setting, and createConnectionString() returns url verbatim without reading the others. An app setting discrete credentials never names url, so a stale url in a config file would survive and silently win — connecting to a different database than the code asked for. Reverted, and pinned with a test.
  • The merge now carries a cycle guard. A framework config slice with a parent back-pointer typechecks fine and used to spin ~5.5s, allocate ~2GB and die with a stack overflow. It is a depth chain popped on exit, not a global seen-set, so the same object legitimately appearing under two keys still merges both times.

Also fixed from review: as const configs assign, the header-bag guard is pinned in the read-back direction that actually breaks, and the dialect-conflict warning no longer names a setConfig call a db.configure caller never made.

Tests

  • src/__tests__/config-options-compile.ts — compile-time, under src/ because test/ is excluded from tsc, so type assertions there would be dead weight.
  • test/config-merge.test.ts + test/config-precedence.test.ts — runtime.

Checked these are real regression cover by running them against the old implementation: 6 of 14 merge tests and both original precedence tests fail there.

Verification

  • bun --bun tsc --noEmit → 0
  • bun test → 1698 pass, 4 skip, 0 fail (baseline 1679, +19 new)
  • bunx --bun pickier . → 0 errors, both at the repo root and inside the package
  • dist builds, loads, and re-exports the new names

A simulated downstream app compiles QueryBuilderOptions with zero restated defaults, while QueryBuilderConfig still correctly reports "missing verbose, snapshotDir, migrationDir, timestamps, and 6 more".

Notes for review

  • The second commit (chore(build)) is unrelated: two pre-existing ts/no-top-level-await errors in build.ts that only surface when pickier runs from inside the package. Suppressed with an explanation rather than restructuring the build, since Bun runs that script directly.
  • Left alone deliberately: the ~22 config.x = { … } doc examples (they still work), and ~179 pre-existing markdown lint warnings in files this PR does not touch.
  • Suggested release: patch, per the 9e99f6a fix(types): allow SQLite-only database config precedent.

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.