Fix type definitions for PersistedModel API #1599
Conversation
Callback's first argument is an optional Error now. Was: required `any`. PersistedModel methods return `PersistedModel` now. Before this change, methods were returning `PersistedData` (`PersistedModel | AnyObject`). The problem with `AnyObject` is that it does not contain any `PersistedModel` instance data and cannot be assigned to functions expecting `Partial<PersistedModel>`. As a result, consumers of this API were forced to either cast the result to `PersistedModel` (which feels wrong) or deal with the `AnyObject` case (which never happen at runtime). Fix definition of `ModelData<T>` to `T | Partial<T>`. Before this change, `ModelData` allowed any values not related to the actual model at all, for example arrays.
bajtos
requested review from
b-admike,
dhmlau,
jannyHou,
shimks,
virkt25 and
zbarbuto
as code owners
types/common.d.ts
Outdated
| @@ -18,7 +18,7 @@ export type Options = AnyObject<any>; | |||
| /** | |||
| * Type alias for Node.js callback functions | |||
| */ | |||
| export type Callback<T = any> = (err: any, result?: T) => void; | |||
| export type Callback<T = any> = (err?: Error | null, result?: T) => void; | |||
There was a problem hiding this comment.
Enable the following usage patterns:
callback()callback(null)
There was a problem hiding this comment.
Shouldn't it be err?: any - AFAIK normally any value passed to first parameter that's not null / undefined means an error. So ... for example, I can pass a string into the Callback?
There was a problem hiding this comment.
Node.js does allow the error parameter to be any.
There was a problem hiding this comment.
Fair enough, I'll change it to err?: any.
types/model.d.ts
Outdated
| @@ -246,4 +246,4 @@ export declare class ModelBuilder extends EventEmitter { | |||
| * Union export type for model instance or plain object representing the model | |||
| * instance | |||
| */ | |||
| export type ModelData<T extends ModelBase = ModelBase> = T | AnyObject; | |||
| export type ModelData<T extends ModelBase = ModelBase> = T | Partial<T>; | |||
There was a problem hiding this comment.
Strangely enough, AnyObject allows arbitrary values not related to ModelBase at all, for example an array. This is a problem in PersistedModel API, where the data argument of methods like create or upsert suddenly allows callers to provide an array, despite the fact that these methods don't support batch mode.
There was a problem hiding this comment.
We probably should introduce a DeepPartial as follows. It's used in TypeORM too.
export type DeepPartial<T> = { [P in keyof T]?: DeepPartial<T[P]>; };
There was a problem hiding this comment.
wouldn't T | Partial<T> be the same as Partial<T>?
There was a problem hiding this comment.
We probably should introduce a DeepPartial as follows. It's used in TypeORM too.
Will do.
wouldn't T | Partial be the same as Partial?
From compiler's point of view, I think it's the same, because T can be assigned to Partial<T>. From documentation perspective, I feel T | Partial<T> is communicating the intent better.
| ): PromiseOrVoid<PersistedModel>; | ||
|
|
||
| static create( | ||
| data: PersistedData[], |
There was a problem hiding this comment.
Add an overload for batch create mode, where an array of data objects is accepted and the method returns an array of model instances created.
|
@raymondfeng @virkt25 @jannyHou Thank you for the feedback, I have addressed your comments in 6f3675b. LGTY now? |
Callback's first argument is an optional Error now. Was: required
any.PersistedModel methods return
PersistedModelnow. Before this change, methods were returningPersistedData(PersistedModel | AnyObject). The problem withAnyObjectis that it does not contain anyPersistedModelinstance data and cannot be assigned to functions expectingPartial<PersistedModel>. As a result, consumers of this API were forced to either cast the result toPersistedModel(which feels wrong) or deal with theAnyObjectcase (which never happen at runtime).Fix definition of
ModelData<T>toT | Partial<T>. Before this change,ModelDataallowed any values not related to the actual model at all, for example arrays.These problems were discovered while working on loopbackio/loopback-next#1475. The fixes presented in this pull request seem to be backwards compatible, at least as far as loopback-next tests are concerned.