CodeDredd / pinia-orm

The Pinia plugin to enable Object-Relational Mapping access to the Pinia Store.
https://pinia-orm.codedredd.de/
MIT License
444 stars 38 forks source link

A few Query methods should have return type this instead of Query<M> #130

Closed 2-5 closed 2 years ago

2-5 commented 2 years ago

Problem

A few Query methods have return type Query<M>, but I think they should have return type this like the other similar methods (where, has, ...)

declare class Query<M extends Model = Model> {
    orderBy(field: OrderBy, direction?: OrderDirection): Query<M>;
    with(name: string, callback?: EagerLoadConstraint): Query<M>;
    withAll(callback?: EagerLoadConstraint): Query<M>;
    withAllRecursive(depth?: number): Query<M>;
}

Context

I'm writing a small adapter layer to be able to easily switch between vuex-orm and pinia-orm until it's clear that I can fully switch to pinia-orm. The relevant part:

orm_pinia_model.ts

import { Model, Query, type Item, type Collection } from "pinia-orm"

import { database } from "@/orm_pinia"

export class ORMModel extends Model {
  static query<M extends typeof ORMModel>(this: M): ORMQuery<InstanceType<M>> {
    return new ORMQuery(database, this.newRawInstance())
  }

  static exists<M extends typeof ORMModel>(this: M): boolean {
    return this.query().exists()
  }

  static all<M extends typeof ORMModel>(this: M): Collection<InstanceType<M>> {
    return this.query().all()
  }

  static find<M extends typeof ORMModel>(this: M, id: string | number): Item<InstanceType<M>> {
    return this.query().find(id)
  }
}

export class ORMQuery<M extends ORMModel> extends Query<M> {
  public exists(): boolean {
    return this.select().length > 0
  }

  public count(): number {
    return this.select().length
  }
}

The problem

The initial query assignment has my adapter type ORMQuery<Item>, but since orderBy returns Query<Item> instead of this, the second assignment fails:

function items(accountId: string, sort: boolean=false): Item[] {
  let query = Item.query()
    .where("account_id", accountId)

  if (sort) {
    // Type 'Query<Item>' is missing the following properties from type 'ORMQuery<Item>': exists, count [ts(2739)]
    query = query.orderBy("account_id")
  }

  return query.get()
}
CodeDredd commented 2 years ago

I already wondered why there are different types at these methods (origin in vuex-orm-next)....well for me it makes sense what you say. I should probably start also including DTS tests.

2-5 commented 2 years ago

@CodeDredd thanks for the quick fix!

That's interesting, it seems they changed from this to Query<M> when going from vuex-orm to vuex-orm-next, but only in some places. It doesn't look like it has a purpose, probably just lack of attention:

https://github.com/vuex-orm/vuex-orm-next/blob/master/src/query/Query.ts#L190

with(name: string, callback: EagerLoadConstraint = () => {}): Query<M> {
  this.eagerLoad[name] = callback

  return this
}

https://github.com/vuex-orm/vuex-orm/blob/master/src/query/Query.ts#L524

with(
  name: string | string[],
  constraint: Contracts.RelationshipConstraint | null = null
): this {
  Loader.with(this, name, constraint)

  return this
}
CodeDredd commented 2 years ago

@2-5 i think that too. anyway thanks for pointing it out and your welcome 😄