Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: improved types #1811

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

psychedelicious
Copy link

Improved types, mostly in Node.ts.

Notes:

  • There should no code changes, only typing changes.
  • I've explicitly typed the return value of many functions that previously had their return types inferred. This provides insurance against unintentional changes to return types.
  • I wrote some docstrings for the export config methods. Please carefully review these for accuracy.

Followups:

  • I'm not sure how the API docs are created. I assume generated from the JSDoc annotations? If so, I can work on those.

@@ -915,8 +915,8 @@ export abstract class Node<Config extends NodeConfig = NodeConfig> {
* @name Konva.Node#getAttrs
* @returns {Object}
*/
getAttrs() {
return (this.attrs || {}) as Config & Record<string, any>;
getAttrs(): Partial<Config> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to type this.attrs as Partial<Config>, but this will require some minor code changes, so I've left it out of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this type, as it limits to config only. But it is possible to write ANY custom data into attributes.

@@ -82,3 +84,148 @@ export interface RGB {
export interface RGBA extends RGB {
a: number;
}

export interface Size {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better name is Dimensions?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to improve the types of this._cache. Currently it is Map<string, any>.

I had a couple ideas to improve this, but I'm not sure of the intended use of the cache. Is it intended to be a generic cache with arbitrary keys, or can we be strict about what keys are used for it?

If we can be strict, I was thinking about a simple wrapper:

const CacheKey = {
  ABSOLUTE_OPACITY: 'absoluteOpacity',
  ALL_LISTENERS: 'allEventListeners',
  ABSOLUTE_TRANSFORM: 'absoluteTransform',
  ... // the rest of the keys
} as const;

type CacheKeyMap = {
  [CacheKey.ABSOLUTE_OPACITY]: number;
  [CacheKey.ALL_LISTENERS]: unknown; // not sure what this should be
  [CacheKey.ABSOLUTE_TRANSFORM]: Transform;
  ... // the rest of the keys & their types
};

class CacheMap extends Map {
  get<T extends keyof CacheKeyMap>(key: T): CacheKeyMap[T] | undefined {
    return super.get(key);
  }
  set<T extends keyof CacheKeyMap>(key: T, value: CacheKeyMap[T]): this {
    return super.set(key, value);
  }
  delete<T extends keyof CacheKeyMap>(key: T): boolean {
    return super.delete(key);
  }
}

This merits discussion - and would require code changes - so I've left it out of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need additional code just for types. Ok, if it is types only, but not if it is a real class.
_cache is not exposed to public API, so as it is very well tested, I don't think we need to care much about its types.

@@ -915,8 +915,8 @@ export abstract class Node<Config extends NodeConfig = NodeConfig> {
* @name Konva.Node#getAttrs
* @returns {Object}
*/
getAttrs() {
return (this.attrs || {}) as Config & Record<string, any>;
getAttrs(): Partial<Config> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this type, as it limits to config only. But it is possible to write ANY custom data into attributes.

@@ -172,11 +183,11 @@ export abstract class Node<Config extends NodeConfig = NodeConfig> {
// all change event listeners are attached to the prototype
}

hasChildren() {
hasChildren(): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don't like having return types everywhere. I prefer less code as it is more clean.

I've explicitly typed the return value of many functions that previously had their return types inferred. This provides insurance against unintentional changes to return types.

It is a good point. But still I prefer not to write type. Probably is is a good idea to write them on complex functions with many returns. But on small one, with one return, most likely we don't really need it. Especially if it is just one direct value return.

@@ -930,7 +930,7 @@ export abstract class Node<Config extends NodeConfig = NodeConfig> {
* fill: 'red'
* });
*/
setAttrs(config: any) {
setAttrs(config?: Partial<Config>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can set any data in attrs. Not just what is inside Config.

quality?: number;
callback?: (str: string) => void;
}) {
toDataURL(config?: ToDataURLConfig): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep this type here directly in the code, so it is not defined somewhere else in the code base. I don't think we reuse this type anywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need additional code just for types. Ok, if it is types only, but not if it is a real class.
_cache is not exposed to public API, so as it is very well tested, I don't think we need to care much about its types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants