Change ModelOrObject to Partial<T> #329
Conversation
As far as I can tell, the functions that accept ModelOrObject parameters (Insert<T>, InsertGraphAndFetch<T>, insertAndFetch<T>. update*, patch, relate) all allow Object as well as Model so that the caller can provide an object that does not define all the properties of the Model. This seems like a weaker type check than is possible with the Partial<T> type that is provided by typescript: https://www.typescriptlang.org/docs/handbook/advanced-types.html#mapped-types This commit changes the ModelOrObject and ModelsOrObjects types to PartialModel and PartialOrPartials types and makes appropriate replacements.
|
Sounds good. @mceachen Do you have any thoughts about this? |
|
This sounds great, I'll review the PR later today. |
| @@ -302,15 +299,14 @@ declare module "objection" { | |||
| export interface QueryBuilder<T> extends QueryBuilderBase<T>, Promise<T[]> { } | |||
|
|
|||
| interface Insert<T> { | |||
| <M extends Model[] | Object[]>(modelsOrObjects?: M): QueryBuilder<T>; | |||
| <M extends ModelOrObject>(modelOrObject?: M): QueryBuilderSingle<T>; | |||
| (modelsOrObjects?: Array<Partial<T>>): QueryBuilder<T>; | |||
There was a problem hiding this comment.
I think it's generally preferred to use Partial<T>[] instead of Array<Partial<T>>.
There was a problem hiding this comment.
Sounds good! Thanks for taking a look
There was a problem hiding this comment.
Also, could you add an example to tests/ts/examples.ts? Something as simple as
// Verify that insert accepts Partial<Person>:
Person.query()
.insert({ firstName: "test" })
.then((p: Person) => console.log("inserted", p))
would be great. Thanks.
|
Let me know what to do about the build failure here. The build fails on two older node versions when compiling a dependency (fs-extra). But, the dependency doesn't support that older node version: Easy solution would be to pin to 2.0.0 but I'll let your team decide on the best solution. |
And jprichardson/node-fs-extra#390 is tracking the fix. I think it's fine to pin to the prior version so they can merge this. |
3 similar comments
|
Should be good to merge. Added the type assertion and pinned to 2.0.0 |
|
Thanks! |
As far as I can tell, the functions that accept ModelOrObject parameters
(Insert, InsertGraphAndFetch, insertAndFetch. update*, patch,
relate) all allow Object as well as Model so that the caller can provide
an object that does not define all the properties of the Model.
This seems like a weaker type check than is possible with the Partial
type that is provided by typescript:
https://www.typescriptlang.org/docs/handbook/advanced-types.html#mapped-types
This commit removes the ModelOrObject and ModelsOrObjects types and replaces
them with Partial and Partial[] where appropriate. This will provide stronger
type checking in the affected calls: callers will no longer be able to set arbitrary
properties in their insert/update/patch calls. They will be restricted to the subset of
properties that exist on the model definition.