Support defining the type when extending a factory with params() #75
Conversation
There was a problem hiding this comment.
Thanks for your contribution. There are a couple issues here that circumvent the type-checker which would prevent me from accepting this as is. We will need a solution that does not do a type coercion (eg. as unknown as TOverride) so that we can guarantee that types are working correctly.
| const factory = this.clone(); | ||
| factory._params = merge({}, this._params, params, mergeCustomizer); | ||
| return factory; | ||
| return factory as unknown as Factory<TOverride, I>; |
There was a problem hiding this comment.
The issue, and I'm not sure yet how to solve it, is that even though TOverride extends T, we aren't guaranteeing that new params are passed in such that the built object is actually a TOverride.
There was a problem hiding this comment.
You are absolutely right, params is Partial and might not contain all properties required for TOverride. 🤔 I also can’t think about any other approach to make it work with the params() method. Maybe with a dedicated extends() method that accepts another generator function.
|
|
||
| it('adds parameters for a sub-type which are then also accepted in build()', () => { | ||
| const adminFactory = userFactory.params<AdminUser>({ | ||
| admin: true, |
There was a problem hiding this comment.
My concern is that if admin: true were not supplied, then the factory happily compiles but does not actually return an AdminUser.
|
Thanks for your feedback. I’m closing the PR as the idea doesn’t work as I thought. |
Fixes: #71