ReviewOS

also looking at this

stacks/bun-query-builder

feat(types): export QueryBuilderOptions, the shape an app actually writes

#1063
Closed glennmichael123 wants to merge feat/query-builder-options into main
2 files +68 -6
11 days ago

Closes #1062.

QueryBuilderConfig is the resolved shape this library reads after merging its own defaults, so all twelve top-level fields are required. It is also what consumers declare their config file against, because it is what the package exports.

Those two roles conflict: every field added to the resolved config is a compile error in every app until each one restates a value the library already supplies. 0.2.25 dropped the ? from migrationDir, and a downstream app had to hard-code 'database/migrations' — defaulted here in three separate places — purely to satisfy the type. snapshotDir did the same one release earlier.

The reviewer's framing was the right one:

nothing in those configs should ever be required and always default to sensible defaults

The change

QueryBuilderOptions = DeepPartial<QueryBuilderConfig> is now the consumer-facing type. Deep rather than shallow matters: plain Partial still demands a complete TimestampConfig from anyone renaming a single column.

QueryBuilderConfig keeps its required fields — internal code should be able to rely on presence. Only the name consumers reach for changes.

setConfig() already accepted a partial, so this names a contract the library was honouring rather than introducing one. applyConfig is threaded through to match.

A real bug the shallow type was hiding

softDeletes and vitess are themselves optional on the resolved config, so merging a partial override over the possibly-absent current value could not guarantee the required members survived:

config.softDeletes = { ...config.softDeletes, ...userConfig.softDeletes }

Shallow Partial masked it by demanding a complete section from the caller. They now seed from defaultConfig, which is also the behaviour an app expects: softDeletes: { enabled: true } should inherit column and defaultFilter rather than dropping them.

Verification

tsc --noEmit clean, 1678 pass / 5 skip / 0 fail across 126 files.

Once released, stacksjs/stacks moves from the interim satisfies Partial<QueryBuilderConfig> (stacksjs/stacks#2301) to satisfies QueryBuilderOptions.

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

Sign in to comment on this pull request.