ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(types)!: type defineModel as the ORM model it returns

#1150
Merged glennmichael123 wants to merge fix/define-model-query-types into main
5 files +288 -7

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/bun-query-builder/src/__tests__/type-usage-compile.tsmodified+55-0
@@ -35,6 +35,7 @@ import type { SelectQueryBuilder, TableRelationName, TypedSelectQueryBuilder } f
3535import type { DatabaseSchema } from '../schema'
3636import { raw } from '../client'
3737import { createModel, type ModelDefinition } from '../orm'
38import { defineModel } from '../model'
3839
3940// ---------------------------------------------------------------------------
4041// 1. Type-assertion helpers
@@ -1222,3 +1223,57 @@ type F3 = Expect<Equal<LedgerFill['currency'], string | undefined>>
12221223// eslint-disable-next-line pickier/no-unused-vars
12231224type F4 = Expect<Equal<LedgerFill['memo'], string>>
12241225const _ledgerInsert: LedgerFill = { memo: 'x', amountCents: 100 }
1226
1227// ---------------------------------------------------------------------------
1228// 9. defineModel() is typed as the ORM model it returns on the server
1229// ---------------------------------------------------------------------------
1230
1231const Trail = defineModel({
1232 name: 'Trail',
1233 table: 'trails',
1234 primaryKey: 'id',
1235 traits: { useTimestamps: true },
1236 attributes: {
1237 name: { type: 'string', fillable: true },
1238 distance: { type: 'number', fillable: true },
1239 },
1240})
1241
1242// eslint-disable-next-line pickier/no-unused-vars
1243async function _defineModelUsage() {
1244 // ORM builder methods that worked at runtime but were typed off the browser model
1245 Trail.query().whereRaw('LOWER(name) = ?', ['ada']).orWhereRaw('distance > ?', 5)
1246 Trail.query().whereGroup(q => q.where('name', 'Ada').orWhere('distance', 5))
1247 Trail.query().where('distance', '>', 1).increment('distance')
1248 const longest: number | null = await Trail.max('distance')
1249 void longest
1250
1251 // Browser-model methods that were typed but missing on the server now exist there
1252 const one = await Trail.query().where('distance', '>', 1).find(1)
1253 if (one) one.get('name')
1254 await Trail.query().findOrFail(1)
1255 Trail.query().latest().oldest('distance')
1256 await Trail.update(1, { name: 'Ridge' })
1257 const deleted: boolean = await Trail.delete(1)
1258 void deleted
1259
1260 // Column names stay narrow
1261 // @ts-expect-error — unknown column
1262 Trail.where('altitude', 1)
1263 // @ts-expect-error — unknown column
1264 Trail.query().orderBy('altitude')
1265
1266 // Build-tool introspection survives
1267 const table: string = Trail.getTable()
1268 const name: 'Trail' = Trail.getName()
1269 const def: 'trails' = Trail.definition.table
1270 void [table, name, def]
1271}
1272
1273// The package root names the ORM builder; DynamoDB's keeps an explicit alias.
1274type RootBuilder = import('../index').ModelQueryBuilder<typeof MemberDef>
1275type OrmBuilder = import('../orm').ModelQueryBuilder<typeof MemberDef>
1276// eslint-disable-next-line pickier/no-unused-vars
1277type RB1 = Expect<Equal<RootBuilder, OrmBuilder>>
1278// eslint-disable-next-line pickier/no-unused-vars
1279type RB2 = Expect<Equal<import('../index').DynamoDBModelQueryBuilder<any>, import('../dynamodb').ModelQueryBuilder<any>>>
packages/bun-query-builder/src/index.tsmodified+5-1
@@ -25,7 +25,11 @@ export * from './types'
2525
2626// Resolve ambiguous re-exports by explicitly choosing which module's version to use
2727export type { WhereOperator } from './browser'
28export type { ModelQueryBuilder } from './dynamodb'
28// The ORM builder that `createModel(...).query()` and `defineModel(...).query()`
29// return. It was the DynamoDB interface, which only the dynamodb entry point
30// should name that; it stays reachable here under an explicit alias.
31export type { ModelQueryBuilder } from './orm'
32export type { ModelQueryBuilder as DynamoDBModelQueryBuilder } from './dynamodb'
2933export type { ColumnName, QueryBuilder } from './client'
3034export { type ModelDefinition, defineModel } from './model'
3135export type { ModelRecord } from './schema'
packages/bun-query-builder/src/model.tsmodified+29-6
@@ -41,7 +41,7 @@
4141 */
4242
4343import { createBrowserModel, isBrowser, type BrowserModelDefinition } from './browser'
44import { clearLocalModels, createModel, type ModelDefinition as OrmModelDefinition } from './orm'
44import { clearLocalModels, createModel, type ModelStatic, type ModelDefinition as OrmModelDefinition } from './orm'
4545
4646// Re-export the browser model types for convenience
4747export type { BrowserModelDefinition as ModelDefinition }
@@ -150,6 +150,27 @@ export function registerModel<TModel>(name: string, model: TModel): TModel {
150150// defineModel
151151// ============================================================================
152152
153/**
154 * The definition as the ORM types read it. A literal definition normally
155 * satisfies the ORM's `ModelDefinition` already, which keeps its column and
156 * relation names narrow; intersecting unconditionally would widen them.
157 */
158type ServerModelDefinition<TDef> = TDef extends OrmModelDefinition ? TDef : TDef & OrmModelDefinition
159
160/**
161 * What `defineModel()` returns: the ORM model, plus the raw definition for
162 * build tools that introspect it.
163 *
164 * It used to be typed as the browser model even though the package root runs
165 * on Bun and hands back the ORM model. So `whereRaw`, `with`, `whereGroup`,
166 * `increment` and the aggregates worked but did not type-check. Browser code
167 * builds its models with `createBrowserModel` from `bun-query-builder/browser`.
168 */
169export type DefinedModel<TDef extends BrowserModelDefinition> = ModelStatic<ServerModelDefinition<TDef>> & {
170 definition: TDef
171 getName: () => TDef['name']
172}
173
153174/**
154175 * Check if we're running in a browser environment
155176 */
@@ -171,12 +192,14 @@ function isClientSide(): boolean {
171192 * @param definition - The model definition
172193 * @returns An isomorphic model with query methods
173194 */
174export function defineModel<const TDef extends BrowserModelDefinition>(definition: TDef) {
175 let model: ReturnType<typeof createBrowserModel<TDef>>
195export function defineModel<const TDef extends BrowserModelDefinition>(definition: TDef): DefinedModel<TDef> {
196 let model: DefinedModel<TDef>
176197
177198 // In browser, use the browser model implementation (fetch-based)
178199 if (isClientSide()) {
179 model = createBrowserModel(definition)
200 // A DOM in a Bun process (happy-dom in tests, say) takes this branch. The
201 // fetch-backed model it gets lacks the server-only builder methods.
202 model = createBrowserModel(definition) as unknown as DefinedModel<TDef>
180203 }
181204 else {
182205 // On server, use the dynamic ORM directly — no code generation needed.
@@ -191,11 +214,11 @@ export function defineModel<const TDef extends BrowserModelDefinition>(definitio
191214 getDefinition: () => definition,
192215 getTable: () => definition.table,
193216 getName: () => definition.name,
194 }) as unknown as ReturnType<typeof createBrowserModel<TDef>>
217 }) as unknown as DefinedModel<TDef>
195218 }
196219
197220 // Register the model in the global registry
198 registerModel(definition.name, model)
221 registerModel(definition.name, model as unknown as ReturnType<typeof createBrowserModel>)
199222
200223 return model
201224}
packages/bun-query-builder/src/orm.tsmodified+50-0
@@ -2773,6 +2773,14 @@ class ModelQueryBuilder<
27732773 return this.orderBy(column, 'asc')
27742774 }
27752775
2776 latest(column: ColumnName<TDef> = 'created_at' as ColumnName<TDef>): ModelQueryBuilder<TDef, TSelected> {
2777 return this.orderBy(column, 'desc')
2778 }
2779
2780 oldest(column: ColumnName<TDef> = 'created_at' as ColumnName<TDef>): ModelQueryBuilder<TDef, TSelected> {
2781 return this.orderBy(column, 'asc')
2782 }
2783
27762784 limit(count: number): ModelQueryBuilder<TDef, TSelected> {
27772785 this._limit = count
27782786 return this
@@ -3197,6 +3205,23 @@ class ModelQueryBuilder<
31973205 return result
31983206 }
31993207
3208 /**
3209 * The row with this primary key that also satisfies the query's other
3210 * constraints, including its soft-delete scope. `defineModel()` models were
3211 * typed with the browser builder's `query().find()`, which compiled on the
3212 * server and then threw because this builder had none.
3213 */
3214 async find(id: number | string): Promise<ModelRecord<TDef, TSelected> | undefined> {
3215 const pk = (this._definition.primaryKey || 'id') as ColumnName<TDef>
3216 return this.where(pk, id as any).first()
3217 }
3218
3219 async findOrFail(id: number | string): Promise<ModelRecord<TDef, TSelected>> {
3220 const result = await this.find(id)
3221 if (!result) throw new Error(`${this._definition.name} with id ${id} not found`)
3222 return result
3223 }
3224
32003225 async last(): Promise<ModelRecord<TDef, TSelected> | undefined> {
32013226 const pk = this._definition.primaryKey || 'id'
32023227 this._orderBy = [{ column: pk, direction: 'desc' }]
@@ -3558,6 +3583,8 @@ export type ModelStatic<TDef extends ModelDefinition> = StaticWhereOverloads<TDe
35583583 createMany: (items: FillableAttributes<TDef>[]) => Promise<ModelRecord<TDef>[]>
35593584 updateOrCreate: (search: Partial<ModelAttributes<TDef>>, data: FillableAttributes<TDef>) => Promise<ModelRecord<TDef>>
35603585 firstOrCreate: (search: Partial<ModelAttributes<TDef>>, data?: FillableAttributes<TDef>) => Promise<ModelRecord<TDef>>
3586 update: (id: number | string, data: FillableAttributes<TDef>) => Promise<ModelRecord<TDef>>
3587 delete: (id: number | string) => Promise<boolean>
35613588 destroy: (id: number | string) => Promise<boolean>
35623589 remove: (id: number | string) => Promise<boolean>
35633590 truncate: () => Promise<void>
@@ -3731,6 +3758,29 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
37313758 return existing || this.create({ ...search, ...data } as any)
37323759 },
37333760
3761 /**
3762 * Update the row with this primary key and return it, running the same
3763 * fill, timestamps and hooks as `instance.update()`. Throws when no such row
3764 * exists. `defineModel()` typed this browser-model method on the server,
3765 * where it was missing.
3766 */
3767 async update(id: number | string, data: FillableAttributes<TDef>): Promise<ModelRecord<TDef>> {
3768 const record = await model.findOrFail(id)
3769 await record.update(data)
3770 return record
3771 },
3772
3773 /**
3774 * Delete the row with this primary key through `instance.delete()`, so a
3775 * soft-deletable model is soft-deleted and delete hooks run. False when no
3776 * such row exists. `defineModel()` typed this browser-model method on the
3777 * server, where it was missing.
3778 */
3779 async delete(id: number | string): Promise<boolean> {
3780 const record = await model.find(id)
3781 return record ? record.delete() : false
3782 },
3783
37343784 async destroy(id: number | string): Promise<boolean> {
37353785 const exec = getExecutor()
37363786 const pk = definition.primaryKey || 'id'
packages/bun-query-builder/test/orm.define-model-server-api.test.tsadded+149-0
@@ -0,0 +1,149 @@
1/**
2 * `defineModel()` is typed as the ORM model it returns on the server.
3 *
4 * It used to carry the browser model's type. The package root runs on Bun and
5 * hands back the ORM model, so the two disagreed in both directions:
6 *
7 * - `whereRaw`, `with`, `whereGroup`, `increment` and the aggregates worked
8 * but did not type-check;
9 * - `Model.update(id, data)`, `Model.delete(id)` and `query().find()`,
10 * `findOrFail()`, `latest()`, `oldest()` type-checked and then threw
11 * `TypeError: ... is not a function`, because the ORM had none of them.
12 *
13 * The types are asserted in src/__tests__/type-usage-compile.ts. This file
14 * covers the ORM methods added so the browser-model calls keep working.
15 */
16
17import { Database } from 'bun:sqlite'
18import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
19import { clearModelRegistry, configureOrm, createTableFromModel, defineModel } from '../src'
20
21let db: Database
22let Trail: any
23let Note: any
24let deletedHook: unknown[]
25
26const TRAILS = [
27 { name: 'Ridge', distance: 12, created_at: '2026-01-02 00:00:00' },
28 { name: 'Creek', distance: 4, created_at: '2026-01-03 00:00:00' },
29 { name: 'Summit', distance: 20, created_at: '2026-01-01 00:00:00' },
30]
31
32beforeEach(async () => {
33 clearModelRegistry()
34 db = new Database(':memory:', { create: true })
35 configureOrm({ database: db })
36 deletedHook = []
37
38 Trail = defineModel({
39 name: 'DmTrail',
40 table: 'dm_trails',
41 primaryKey: 'id',
42 autoIncrement: true,
43 attributes: {
44 name: { type: 'string', fillable: true },
45 distance: { type: 'number', fillable: true },
46 created_at: { type: 'string', fillable: true },
47 },
48 } as any)
49 Note = defineModel({
50 name: 'DmNote',
51 table: 'dm_notes',
52 primaryKey: 'id',
53 autoIncrement: true,
54 traits: { useSoftDeletes: true },
55 attributes: { body: { type: 'string', fillable: true } },
56 hooks: { afterDelete: (note: any) => { deletedHook.push(note.id) } },
57 } as any)
58
59 await createTableFromModel(Trail.getDefinition())
60 await createTableFromModel(Note.getDefinition())
61 for (const t of TRAILS) await Trail.create(t)
62 await Note.create({ body: 'first' })
63 await Note.create({ body: 'second' })
64})
65
66afterEach(() => {
67 clearModelRegistry()
68 db.close()
69})
70
71describe('query().find() and findOrFail()', () => {
72 it('finds by primary key within the query constraints', async () => {
73 expect((await Trail.query().find(2))?.get('name')).toBe('Creek')
74 expect(await Trail.query().where('distance', '>', 10).find(2)).toBeUndefined()
75 expect((await Trail.query().where('distance', '>', 10).find(3))?.get('name')).toBe('Summit')
76 })
77
78 it('respects the soft-delete scope', async () => {
79 await Note.delete(1)
80 expect(await Note.query().find(1)).toBeUndefined()
81 expect((await Note.query().withTrashed().find(1))?.get('body')).toBe('first')
82 })
83
84 it('findOrFail throws for a missing row', async () => {
85 expect((await Trail.query().findOrFail(1)).get('name')).toBe('Ridge')
86 await expect(Trail.query().where('distance', '<', 5).findOrFail(1)).rejects.toThrow('DmTrail with id 1 not found')
87 })
88})
89
90describe('query().latest() and oldest()', () => {
91 it('order by created_at by default', async () => {
92 expect((await Trail.query().latest().get()).map((t: any) => t.get('name'))).toEqual(['Creek', 'Ridge', 'Summit'])
93 expect((await Trail.query().oldest().get()).map((t: any) => t.get('name'))).toEqual(['Summit', 'Ridge', 'Creek'])
94 })
95
96 it('take an explicit column', async () => {
97 expect((await Trail.query().latest('distance').first())?.get('name')).toBe('Summit')
98 expect((await Trail.query().oldest('distance').first())?.get('name')).toBe('Creek')
99 })
100})
101
102describe('Model.update(id, data)', () => {
103 it('updates the row and returns it', async () => {
104 const updated = await Trail.update(2, { distance: 5 })
105 expect(updated.get('distance')).toBe(5)
106 const row = db.query('SELECT distance FROM dm_trails WHERE id = 2').get() as { distance: number }
107 expect(row.distance).toBe(5)
108 expect((db.query('SELECT distance FROM dm_trails WHERE id = 1').get() as { distance: number }).distance).toBe(12)
109 })
110
111 it('throws for a missing row', async () => {
112 await expect(Trail.update(99, { distance: 5 })).rejects.toThrow('DmTrail with id 99 not found')
113 })
114})
115
116describe('Model.delete(id)', () => {
117 it('deletes the row', async () => {
118 expect(await Trail.delete(2)).toBe(true)
119 expect((db.query('SELECT id FROM dm_trails ORDER BY id').all() as { id: number }[]).map(r => r.id)).toEqual([1, 3])
120 })
121
122 it('soft-deletes a soft-deletable model and runs delete hooks', async () => {
123 expect(await Note.delete(1)).toBe(true)
124 const row = db.query('SELECT deleted_at FROM dm_notes WHERE id = 1').get() as { deleted_at: string | null }
125 expect(row.deleted_at).not.toBeNull()
126 expect(deletedHook).toEqual([1])
127 })
128
129 it('returns false for a missing or already-trashed row', async () => {
130 expect(await Trail.delete(99)).toBe(false)
131 await Note.delete(1)
132 expect(await Note.delete(1)).toBe(false)
133 })
134})
135
136describe('server-only builder methods on a defineModel() model', () => {
137 it('whereRaw, whereGroup and increment run', async () => {
138 const rows = await Trail.query().whereRaw('LOWER(name) = ?', ['ridge']).get()
139 expect(rows.map((t: any) => t.id)).toEqual([1])
140 await Trail.query().whereGroup((q: any) => q.where('name', 'Ridge').orWhere('name', 'Creek')).increment('distance', 1)
141 expect((db.query('SELECT distance FROM dm_trails ORDER BY id').all() as { distance: number }[]).map(r => r.distance)).toEqual([13, 5, 20])
142 })
143
144 it('keeps the introspection helpers', () => {
145 expect(Trail.definition.table).toBe('dm_trails')
146 expect(Trail.getTable()).toBe('dm_trails')
147 expect(Trail.getName()).toBe('DmTrail')
148 })
149})