ReviewOS

stacks/bun-query-builder

Config type forces apps to restate defaults: every added field is a downstream compile error

#1062
Closed glennmichael123 opened this 22 days ago · 0 comments
22 days ago

QueryBuilderConfig is the shape the library reads after merging its own defaults, so all twelve of its top-level fields are required:

REQUIRED  verbose, dialect, database, snapshotDir, migrationDir, timestamps,
          pagination, aliasing, relations, transactionDefaults, sql, features
optional  browser, debug, hooks, softDeletes, sqlite, vitess

It is also, in practice, the type 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.

What it looked like downstream

0.2.25 declared migrationDir: string without the optional marker (types.d.ts:233). bun x tsc --noEmit in stacksjs/stacks went from exit 0 to one error, and the fix was to hard-code into the app config the exact string the library already defaults to:

migrationDir: 'database/migrations',

That value has three defaults in this package already:

  • defaultConfig.migrationDir = 'database/migrations' (src/config.ts:21)
  • getSqlDirectory(workspaceRoot?, migrationDir = 'database/migrations') (src/workspace.ts:38)
  • const configured = migrationDir || 'database/migrations' (src/workspace.ts:39)

so the app was duplicating a default three times over to satisfy a type. snapshotDir went the same way one release earlier.

The reviewer's verdict, which is the right framing:

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

Suggested fix

setConfig() already takes Partial<QueryBuilderConfig> (src/config.ts:301), so the library's own contract for user input is partial. Export that as the consumer-facing type rather than leaving apps to find it:

export type QueryBuilderOptions = DeepPartial<QueryBuilderConfig>

Deep rather than shallow matters: Partial alone still forces a complete TimestampConfig on anyone who sets one field of it.

Keeping QueryBuilderConfig as the resolved type is right — internal code should be able to rely on presence. It is only the name consumers reach for that needs to be the partial one.

Downstream mitigation in the meantime is satisfies Partial<QueryBuilderConfig> (stacksjs/stacks#2301), which works against 0.2.25 today, but every other consumer will hit this on the next added field.

Sign in to comment on this issue.