feat: Factory types #1812
feat: Factory types #1812
Conversation
| @@ -2660,7 +2660,7 @@ export abstract class Node<Config extends NodeConfig = NodeConfig> { | |||
| rotation: GetSet<number, this>; | |||
| zIndex: GetSet<number, this>; | |||
|
|
|||
| scale: GetSet<Vector2d | undefined, this>; | |||
| scale: GetSet<Vector2d, this>; | |||
There was a problem hiding this comment.
Either this GetSet type is incorrect for the NodeConfig.scale type is incorrect. I assume it's this one.
There was a problem hiding this comment.
Looks like it is not correct, I don't think scale can return undefined.
| @@ -3102,7 +3102,7 @@ addGetterSetter(Node, 'offsetY', 0, getNumberValidator()); | |||
| * node.offsetY(3); | |||
| */ | |||
|
|
|||
| addGetterSetter(Node, 'dragDistance', null, getNumberValidator()); | |||
| addGetterSetter(Node, 'dragDistance', undefined, getNumberValidator()); | |||
There was a problem hiding this comment.
This is a subtle API change.
Perhaps we should widen the type of dragDistance in NodeConfig to dragDistance?: number | null;?
There are a number of other places where the initial value is set to null apparently erroneously.
There was a problem hiding this comment.
The drag distance must be always number. So undefined should fallback to default value. I am not really sure what it was null here.
| strokeLinearGradientStartPointX: GetSet<number, this>; | ||
| strokeLinearGradientStartPointY: GetSet<number, this>; | ||
| strokeLinearGradientEndPointX: GetSet<number, this>; | ||
| strokeLinearGradientEndPointY: GetSet<number, this>; | ||
| fillRule: GetSet<CanvasFillRule, this>; |
There was a problem hiding this comment.
These annotations were missing. I don't think this changes anything because the Factory adds the GetSets anyways.
There was a problem hiding this comment.
I think I have understood the intention with these validators - they should create warnings but otherwise let the value through, even if it is invalid.
| pointerDirection: GetSet< | ||
| 'left' | 'top' | 'right' | 'bottom' | typeof NONE, | ||
| this | ||
| >; |
There was a problem hiding this comment.
The factory sets the initial value to NONE.
| Factory.addGetterSetter(Transformer, 'node'); | ||
|
|
There was a problem hiding this comment.
I think this was added accidentally - Transformer never references this.node as far as I can see...
There was a problem hiding this comment.
This is an old (deprecated) API. Should be safe to remove.
| addGetterSetter(constructor, attr, def?, validator?, after?) { | ||
| addGetterSetter<T extends Constructor, U extends Attr<T>>( | ||
| constructor: T, | ||
| attr: U, |
There was a problem hiding this comment.
We can make this a bit safer with a type to constrain attr to be a GetSet.
type EnforceGetSet<
T extends Constructor,
U extends Attr<T>
> = InstanceType<T>[U] extends GetSet<any, any> ? U : never;Then, type attr as attr: EnforceGetSet<T, U>.
However, this causes a problem in Canvas.ts at https://github.com/psychedelicious/konva/blob/9c41a6eed1bba46e80ce282903ac86bb11493e84/src/Canvas.ts#L171
|
For this PR, I was just kinda having fun exploring typescript to see if I could get autocomplete working for the Factory methods. Because it is an internal API, it's not a big deal if it has perfect type safety or not - I'm not dead-set on this PR being accepted. |
| @@ -79,7 +149,7 @@ export const Factory = { | |||
| key; | |||
|
|
|||
| if (validator) { | |||
| val = validator.call(this, val); | |||
| val = validator.call(this, val, attr); | |||
| @@ -3102,7 +3102,7 @@ addGetterSetter(Node, 'offsetY', 0, getNumberValidator()); | |||
| * node.offsetY(3); | |||
| */ | |||
|
|
|||
| addGetterSetter(Node, 'dragDistance', null, getNumberValidator()); | |||
| addGetterSetter(Node, 'dragDistance', undefined, getNumberValidator()); | |||
There was a problem hiding this comment.
The drag distance must be always number. So undefined should fallback to default value. I am not really sure what it was null here.
| Factory.addGetterSetter(Transformer, 'node'); | ||
|
|
There was a problem hiding this comment.
This is an old (deprecated) API. Should be safe to remove.
| @@ -551,7 +551,7 @@ Factory.addGetterSetter(TextPath, 'text', EMPTY_STRING); | |||
| * // underline text | |||
| * shape.textDecoration('underline'); | |||
| */ | |||
| Factory.addGetterSetter(TextPath, 'textDecoration', null); | |||
| Factory.addGetterSetter(TextPath, 'textDecoration', undefined); | |||
There was a problem hiding this comment.
This probably should be an empty string as in Text class.
Add types to the Factory helpers & validators.
Notes: