polotno-project / polotno-board

Roadmap and bug-tracker for the Polotno project.
https://polotno.dev/
9 stars 1 forks source link

Better TypeScript support please. Remove `any` type, use generics well, and export useful types. #83

Open ADTC opened 2 months ago

ADTC commented 2 months ago

Polotno SDK seems to have subpar TypeScript support. The type system seems to be added on as an afterthought and isn't robust or always helpful. Often it hinders development because I have to fight with the confusing type system.

I hope this can be rectified by ensuring Polotno SDK has excellent TypeScript support.

I'd completely replace all instances of any with the actual types, or generics. I would also export more of the useful types instead of locking them up locally.

For example: the ImageGrid component should have generics: (the ImageType is the generic)

// Note that it shouldn't be a const but a function. Also there's no need to expand all the props.
export declare function ImagesGrid<ImageType>(props: Props<ImageType>): React.JSX.Element;

Simply doing this alone will automagically type all the props of ImageGrid correctly. If an array of specific type is passed in to images:

image

Then the callback functions like getPreview, getCredit etc. would show the correct type in the function parameter:

image

This is very helpful when writing the callback function code, as we can be type-safe in how we're accessing the incoming function parameter object.

Right now, this isn't possible because the type would just be any and the most I can do is to manually assert the correct type.

PS: There's an ESLint rule @typescript-eslint/no-explicit-any which I enable to warn me of all usages of any. It's possible to ignore it on a per-line or per-file basis if needed, but that should be seldom used.

lavrton commented 2 months ago

Please upgrade to the last version. It should work better.

ADTC commented 2 months ago

Thank you. It works better as the callbacks in ImagesGrid are typed correctly with the images array's element type. I also appreciate that the Section type is now exported.

Hope these improvements continue and there's stronger typing where possible, reducing the usage of any to only instances where the type is truly unknown to you.


One example that stuck in my mind (and will need quite a bit of work) is the use of a generic Element type which contains the common properties for all elements, and then more specific elements like ImageElement and TextElement will take Element type as the base type and superimpose additional properties specific to them.

Then it will be possible to type store.pages.children as IArrayType<Element> instead of IArrayType<any>. (PS: The Beta Custom Elements feature will need to ensure custom elements also use Element type as their base.)

(PS: I noticed there's already ElementType in group-model but it isn't shared as I mentioned.)


Another is whether custom property can be typed as custom?: { [key: string]: any } instead custom: any.

But this would be a breaking change for devs who ignored docs and put non-compliant data in custom property (e.g. custom: "string value"). So maybe it's debatable and perhaps best avoided at this point, and announced as a breaking change in the next major or minor version.


Thank you again, for accommodating so many requests from me. 🙂

PS: One more: element in onSelect in type Props<ImageType> should have ElementType, not ShapeType.