Conversation
Adding typescript types
index.d.ts
Outdated
| type Constructor<T = {}> = new (...args: any[]) => T; | ||
|
|
||
| export default function (options?: { | ||
| identifiers?: string[]; |
There was a problem hiding this comment.
Looking at this line, we are validating if any of the provided properties (identifiers or fields) are empty, so maybe you should define both properties as required instead of optional (since they cannot be empty anyway).
There was a problem hiding this comment.
Definitely, I made options, identifiers and fields required.
index.d.ts
Outdated
|
|
||
| export default function (options?: { | ||
| identifiers?: string[]; | ||
| fields?: string[]; |
There was a problem hiding this comment.
As you can read in the README example, the compound fields can be specified as an Array. So the type for the fields property should be Array<string | string[]>.
There was a problem hiding this comment.
I should I read more, I changed it to accept also an array of strings
index.d.ts
Outdated
| export default function (options?: { | ||
| identifiers?: string[]; | ||
| fields?: string[]; | ||
| }): <T extends typeof Model>(model: T) => T & Constructor<T> |
There was a problem hiding this comment.
Taking in account the compatibility with other objection plugins, the return type should be just the provided model.
Example: https://github.com/oscaroox/objection-timestamps/blob/master/src/index.ts#L25.
There was a problem hiding this comment.
I followed objection timestamp implementation to return the model itself.
Thanks for all the feedback!
- Made Options, identifiers and fields required - Extended types accepted by identifiers and fields to allow for string or array of strings - Changed the returned value to be the model itself and not a Constructor of it
|
Hello, any update on this ? :) |
index.d.ts
Outdated
| import { Model } from 'objection'; | ||
|
|
||
| export default function (options: { | ||
| identifiers: Array<string | string[]>; |
There was a problem hiding this comment.
Oh, this is odd. identifiers is not technically required, since it defaults to ["id"]:
options = Object.assign({
identifiers: ['id']
}, options);Otherwise looks good, works fine.
There was a problem hiding this comment.
Yeah, you are right. If you don't set the options.identifiers, the validation I talked before won't fail because it has the default value ['id']. It just will fail in case you set identifiers as [] or not set the fields option. So you can change identifiers to be optional, but keep the fields as required, mb :)
There was a problem hiding this comment.
Hello, I made the identifiers optional
Made identifier optional
3 participants
Adding typescript types
Types where missing :)