Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Objection2] Documentation request for plugin in TypeScript #1589

Closed
willsoto opened this issue Dec 3, 2019 · 15 comments · Fixed by #1637
Closed

[Objection2] Documentation request for plugin in TypeScript #1589

willsoto opened this issue Dec 3, 2019 · 15 comments · Fixed by #1637

Comments

@willsoto
Copy link
Contributor

willsoto commented Dec 3, 2019

There is currently a "recipe" of how to write a plugin (essentially a mixin) using regular JavaScript but I think it would be nice to have a TypeScript example just like you do for the query builder.

@koskimas
Copy link
Collaborator

koskimas commented Dec 3, 2019

It's basically impossible to type plugins unfortunately. Especially if multiple plugins are used. There's no way to extend an existing dynamic type in typescript. A plugin is a function that takes a model class constructor and returns a new constructor inherited from the input class:

function somePlugin<T extends Model>(modelClass: ModelClass<T>): ??? {
  return class extends modelClass {
    ...
  }
}

The ??? part is impossible to type so that the plugin works for both

class Person extends somePlugin(Model) {

}

and

class Person extends somePlugin(someOtherPlugin(Model)) {

}

If the plugin doesn't add any new methods to neither the QueryBuilder nor the Model, the typing is trivial: just return the input type:

function somePlugin<T extends typeof Model>(modelClass: T): T {
  ...
}

@willsoto
Copy link
Contributor Author

willsoto commented Dec 3, 2019

That is, unfortunately, what I have come to realize after many hours of trying to type them. Even after lowering my expectations and peppering a few @ts-ignores around, it doesn't seem doable in a way that isn't totally gross. I was hoping with the addition of mixin support in TS it would be doable, but it does seem like anything that is non-trivial is just not doable cleanly.


As an aside, one thing I was using with some level of success before was ModelClass, which now seems to be an empty interface.

Is it no longer required to have that duplication? I believe it was there before to deal with static class members.

@koskimas
Copy link
Collaborator

koskimas commented Dec 3, 2019

The old ModelClass is now replaced with typeof Model in most places.

@willsoto
Copy link
Contributor Author

willsoto commented Dec 3, 2019

That is what I thought. Thanks.

I'd like to write some docs anyway with some examples along with the drawbacks. Would that be okay?

@koskimas
Copy link
Collaborator

koskimas commented Dec 3, 2019

hmm... I was able to create this.. Maybe it's possible nowadays after all

import { Model, QueryBuilder, Page } from './typings/objection';

type Constructor<T extends Model> = new (...args: any[]) => T;

class CustomQueryBuilder<M extends Model, R = M[]> extends QueryBuilder<M, R> {
  ArrayQueryBuilderType!: CustomQueryBuilder<M, M[]>;
  SingleQueryBuilderType!: CustomQueryBuilder<M, M>;
  NumberQueryBuilderType!: CustomQueryBuilder<M, number>;
  PageQueryBuilderType!: CustomQueryBuilder<M, Page<M>>;

  someCustomMethod(): this {
    return this;
  }
}

function mixin<T extends Constructor<Model>>(ModelClass: T) {
  return class extends ModelClass {
    static QueryBuilder = QueryBuilder;
    QueryBuilderType: CustomQueryBuilder<this, this[]>;

    mixinMethod() {}
  };
}

class Person extends Model {}

const MixinPerson = mixin(Person);

async () => {
  const z = await MixinPerson.query()
    .whereIn('id', [1, 2])
    .someCustomMethod()
    .where('foo', 1)
    .someCustomMethod();

  z[0].mixinMethod();
};

@koskimas
Copy link
Collaborator

koskimas commented Dec 3, 2019

This was with typescript 3.7.2

@willsoto
Copy link
Contributor Author

willsoto commented Dec 3, 2019

That is actually really close to what I have. I'll update my stuff to reflect your example and let you know if there are any issues. I am also on 3.7.2.

@willsoto
Copy link
Contributor Author

willsoto commented Dec 3, 2019

So here is the example I am working with. I'll annotate with the errors I am seeing:

import { Model } from 'objection';

export type ModelConstructor<T extends Model = Model> = new (
  ...args: any[]
) => T;

export function Mixin(options = {}) {
  return function<T extends ModelConstructor>(Base: T) {
    class CustomQueryBuilder<M extends Model, R = M[]> extends QueryBuilder<M, R> {
      ArrayQueryBuilderType!: CustomQueryBuilder<M, M[]>;
      SingleQueryBuilderType!: CustomQueryBuilder<M, M>;
      NumberQueryBuilderType!: CustomQueryBuilder<M, number>;
      PageQueryBuilderType!: CustomQueryBuilder<M, Page<M>>;

      // Class 'QueryBuilder<M, R>' defines instance member property 'delete', but extended class 'CustomQueryBuilder<M, R>' defines it as instance member function.ts(2425)
      delete() {
        const value = new Date();

        // Argument of type '{ [x: string]: unknown; }' is not assignable to parameter of type 'PartialModelObject<this["ModelType"]>'.ts(2345)
        return this.patch({
          "deleted_at": value
        });
      }

      kept(): this {
        // Property 'ref' does not exist on type 'T'.ts(2339)
        return this.whereNull(Base.ref(column));
      }

      discarded(): this {
        // Property 'ref' does not exist on type 'T'.ts(2339)
        return this.whereNotNull(Base.ref(column));
      }
    }

    return class extends Base {
      static QueryBuilder = CustomQueryBuilder;

      QueryBuilderType!: CustomQueryBuilder<this, this[]>;

      static get modifiers(): Modifiers<CustomQueryBuilder<Model>> {
        return {
          // Property 'modifiers' does not exist on type 'T'.ts(2339)
          ...super.modifiers,
          discarded(builder) {
            builder.discarded();
          },
          kept(builder) {
            builder.kept();
          }
        };
      }
    };
  };
}

Based on these errors, it feels like TypeScript just can't figure out the correct type for T here. It can't figure out any static or instance properties. When I type Base. this is the autocomplete I get - basically just function methods and properties. It has no additional Objection properties or methods.

Screen Shot 2019-12-03 at 2 29 58 PM

@koskimas
Copy link
Collaborator

koskimas commented Dec 3, 2019

The ModelConstructor doesn't have any properties or methods so that's completely understandable. How about

export function Mixin(options = {}) {
  return function<T extends typeof Model>(Base: T) {

@willsoto
Copy link
Contributor Author

willsoto commented Dec 3, 2019

I was using T extends typeof Model before your example, and that got rid of most of the errors there. The remaining errors were these two:

// Class 'QueryBuilder<M, R>' defines instance member property 'delete', but extended class 'CustomQueryBuilder<M, R>' defines it as instance member function.ts(2425)
delete() {
  const value = new Date();

  // Argument of type '{ [x: string]: unknown; }' is not assignable to parameter of type 'PartialModelObject<this["ModelType"]>'.ts(2345)
  return this.patch({
    "deleted_at": value
  });
}

The second error ts(2345) makes sense to me I think, but I am not sure how to resolve it.

@koskimas
Copy link
Collaborator

koskimas commented Dec 6, 2019

The first error is now fixed in 2.0.3

@willsoto
Copy link
Contributor Author

willsoto commented Dec 6, 2019

Thank you!

I ended up doing an assertion for the second error:

this.patch({
  "deleted_at": value
} as PartialModelObject<T>);

Not the worst compromise to me...

@koskimas
Copy link
Collaborator

koskimas commented Dec 9, 2019

That error makes complete sense. There is no deleted_at property. You can define it for your model, then you shouldn't need the cast

@willsoto
Copy link
Contributor Author

willsoto commented Dec 9, 2019

@koskimas Yes, you are right. I got mixed up since I simplified it for the example. In the code I am using, that column is dynamic based on options passed to the decorator. And I have yet to find a way to dynamically add properties to classes in that way via a decorator.

@olavim
Copy link

olavim commented Jan 9, 2020

Fyi @koskimas, generating declaration files ("declaration": true in tsconfig.json) with the examples discussed here does not work, because CustomQueryBuilder must be exported. I've gone into further detail here if you want to give your opinion/insight: olavim/objection-cursor#12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants