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

Types for query methods #247

Merged
merged 12 commits into from
May 12, 2020
Merged

Conversation

ggurkal
Copy link
Contributor

@ggurkal ggurkal commented May 6, 2020

This is the only thing I could think of for query methods types. Using mongoose's own types for Model's QueryHelpers, which is added as the third type argument to the getModelForClass function.

@hasezoey
Copy link
Member

hasezoey commented May 6, 2020

this is more or less exactly what typegoose tries to "prevent" (making interfaces for mongoose schemas/models)


otherwise, the only think i would like to change is to swap the generics of getModelForClass to be more user friendly definable

@hasezoey hasezoey added dont merge yet Dont merge this PR *yet* feature Adds a new Feature or Request labels May 6, 2020
@ggurkal
Copy link
Contributor Author

ggurkal commented May 6, 2020

Yes, I know and I don't really like it either but otherwise there are no types at all for people who'd like to have them.

I'll investigate it a bit further to see if it's possible to have types without defining interfaces explicitly.

@hasezoey
Copy link
Member

hasezoey commented May 6, 2020

I'll investigate it a bit further to see if it's possible to have types without defining interfaces explicitly.

because if we would make the functions in the class again, we would need an instanceMethod and queryMethod decorators and would have to filter them out again and when making it outside of the class (and class-level-decorators like currently), then there are no types automatically, so the best option is to use an interface (to define one or more query functions)
-> so i guess you dont need to search for this, because there will not be an other simpler way

@ggurkal
Copy link
Contributor Author

ggurkal commented May 9, 2020

I have two easier solutions/proposals and assume we have the following in both.

function findByName(this: ReturnModelType<typeof MyClass, MyClass, SchemaQueries>, name: string) {
  return this.find({ name });
}

function findByLastname(this: ReturnModelType<typeof MyClass, MyClass, SchemaQueries>, lastname: string) {
  return this.find({ lastname });
}

@queryMethod(findByName)
@queryMethod(findByLastname)
class MyClass {
  @prop()
  public name: string;

  @prop()
  public lastname: string;
}

Usage:

const MyModel = getModelForClass<MyClass, typeof MyClass, SchemaQueries>(MyClass);

  1. Easier interface
// Will be placed in `types.ts`
type QueryMethod<T extends (...args: any) => any> = (...args: Parameters<T>) => ReturnType<T>;

// In model's file
interface SchemaQueries {
  findByName: QueryMethod<typeof findByName>;
  findByLastname: QueryMethod<typeof findByLastname>;
}
  1. Defining functions in an object.
// Will be placed in `types.ts`
type QueryMethodTypes<T extends {}> = {
  [F in keyof T]: T[F] extends (...args: any) => any ? (...args: Parameters<T[F]>) => ReturnType<T[F]> : never;
}

// In model's file
const MyMethods = { findByName, findByLastname };
type SchemaQueries = QueryMethodTypes<typeof MyMethods>;

@hasezoey
Copy link
Member

hasezoey commented May 9, 2020

i think solution 1 is better and easier to understand & modify (and without any runtime extras)

@ggurkal
Copy link
Contributor Author

ggurkal commented May 9, 2020

I agree.

Do you think we should make QueryHelpers type the first type in getModelForClass ?

@hasezoey
Copy link
Member

hasezoey commented May 9, 2020

Do you think we should make QueryHelpers type the first type in getModelForClass ?

yes, but this would be an breaking change (maybe even major)

but on the other side, the types that would need to be before it would need to be defined - which is not good because they should work automatically
-> i wanted to change up the types anyway

@ggurkal
Copy link
Contributor Author

ggurkal commented May 10, 2020

@hasezoey are you planning to merge this PR or should we make it specific to updating the related types in getModelForClass and keep it until they’re ready?

@hasezoey
Copy link
Member

are you planning to merge this PR

i still need to check out this pr locally, and if all works, then i have some changes in mind i want to test & integrate

@hasezoey
Copy link
Member

@ggurkal are you ok with the changes i made?

src/types.ts Show resolved Hide resolved
@hasezoey hasezoey added this to the 7.1 milestone May 12, 2020
Since types are handled correctly, comments related to query methods are removed.
@ggurkal
Copy link
Contributor Author

ggurkal commented May 12, 2020

@ggurkal are you ok with the changes i made?

Yes

@hasezoey hasezoey merged commit be0d74e into typegoose:master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont merge yet Dont merge this PR *yet* feature Adds a new Feature or Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Query Methods Type Tracking
2 participants