Fix toObject TS issues

[jiayihu]

For some obscure reasons, the current complex TS types for toObject makes a perfectly fine instance of Textbox not assignable to IText because of incompatibility in toObject.

After simplifying the types like this, it was still not assignable but this type is easier to reason about.

• An object of type Textbox is assignable to a parameter of type IText if every property and method is assignable, i.e. every property and method of Textbox is subtype of IText
• Textbox.prototype.toObject is subtype of IText.prototype.toObject if:
  1. The parameter type propertiesToInclude from IText is assignable to the one from Textbox. Notice the change of direction, this is couter-intuitive. But indeed for Textbox.prototype.toObject to be used in place of IText.prototype.toObject, it means that it must be safely accept the parameters from IText.prototype.toObject
  2. The return type SProps from Textbox is assignable to the return type from IText. This is intuitive.

The issue with the above simplified type is that SProps from Textbox can be different from SProps from IText, even though apparently the one from Textbox seems to extend the one in IText in the default case. E.g.

interface SPropsTextbox extends SerializedTextProps { a: string }
interface SPropsIText extends SerializedTextProps { b: string }

The above two types are incompatible even though they seem both to extend from SerializedTextProps. And this issue applies both to the parameter type and the return type of toObject. I think this also explains why the current overcomplex type T extends Omit<Props & TClassProperties<this>, keyof SProps> just makes everything worse and for some reason TS seem even to just give up and say that K extends keyof T is just string | number | symbol for propertiesToInclude from IText (not shown in the above screenshot because it contained only the beginning of the huge TS error). string | number | symbol is the default type returned from keyof T when type T = object, so it's like as if TS just said that T extends Omit<Props & TClassProperties<this>, keyof SProps> for IText is just any generic object.

So my proposal is to simplify the types for toObject, which I don't expect to be that much called, except for serialization. So the current attempt to be overly-accurate in the TS is counter-effective. And if you want to customize the types, you can just do declaration merging, e.g. extend the interface of SerializedTextProps to add extra properties.

The advantage now is also that, because we're not doing SProps extends SerializedTextProps, it's not possible to have a SProps type from Textbox that is incompatible with the SProps from IText. The toObject method statically expects a keyof SerializedTextboxProps, which is guaranteed to extend from SerializedTextProps so TS can easily verify that SerializedTextProps -> (assignable) SerializedTextboxProps for the parameter type and SerializedTextboxProps -> SerializedTextProps for the return type, thus Textbox.prototype.toObject -> IText.prototype.toObject

I think this should significantly also improve type checking performance, since TS doesn't have to do all the Omit<Props & TClassProperties<this>, keyof SProps> computation and compare if one SProps is assignable to the other.

[asturur]

@jiayihu i think SProps was added in the constructor for this reason.

I can add a property name to rect.
So i do:

const r = new Rect({ name: 'test' });

then when i export to toObject, i want propertiesToInclude to recognize name as a valid one.

Since serializedProps is an interface, i can still use the merge property of interfaces and make that work, correct? i just need to do it outside the class declaration.

instead of doing:

const r = new Rect<A , B, C>({ name: 'test' });

i will do

import { SerializedRectProps } from ...

interface SerializedRectProps {
  // add name to all rects?
  name: 'string';
}
const r = new Rect<A , C>({ name: 'test' });

Is this correct?

The only difference is that i would act on all the the Rect classes since the interface merge i think is global?
I don't have opinions on that.

Please explain me as i i was a 5 years old because i m still learning TS

[ShaMan123]

@jiayihu I told you AssertKeys was just the beginning.
This won't end easily.
And should be thought of as a whole.
TS doesn't respect keyof this when subclassing.

[ShaMan123]

IMO we don't need declaration merging nor ClassProperties for a subclass, we just pass SProps.
If we are not subclassing then we are in need of one of these.
But you could declare an interface for declaration merging, extending the superclass, and passing it a different SProps, then it should not be global.
But TS will bother you about that as well.
I am accepting (not with great pleasure) the fact that TS is noisy.
It should make our lives easier. I do not want to work for it, it is slavery.
I want it to work for me. And if it can't I silence it.
But of course that can happen on my project, fabric must be clean of issues.

[ShaMan123]

I am not sure omitting SProps is the right way to go.
Did you try only removing the usage of keyof this?
We had the exact same problem when trying to type set, _set

[ShaMan123]

This makes no sense
You would not pass keus of serialized props because they are already there.
I undrrstand you are thinking of declaration merging but still

[asturur]

I am not sure omitting SProps is the right way to go. Did you try only removing the usage of keyof this? We had the exact same problem when trying to type set, _set

let's try to flesh out why removin SProps is not a good idea so that i can follow the reasoning.
In general the usage of of this in TS seems problematic with subclassing to me

[jiayihu]

Did you try only removing the usage of keyof this?

See my initial post, I tried with a simplified type already and explained why any type parameter would break assignability of subclasses.

toObject<
    SProps extends SerializedITextProps = SerializedITextProps,
    K extends keyof SProps = never
  >(propertiesToInclude: K[] = []): SProps;

In the above simplified type, both the parameters are not assignable (propertiesToInclude (IText) -> propertiesToInclude (Textbox)) and the return types are not assignable SProps (Textbox) -> SProps (IText). That's because if you allow any type parameter SProps, it means that you can technically have SPropsTextbox !== SPropsIText even though Textbox extends IText.

And if it can't I silence it.

You can't really expect people to continuously silence something common such as assigning a Textbox instance to a function parameter expecting IText. I believe that losing some type strictness over something like toObject is preferable compared to asking people to silence errors when passing an instance around to functions.

@jiayihu I told you AssertKeys was just the beginning.
This won't end easily.

Actually it seems, at least for now, that AssertKeys + toObject were indeed the two things that didn't make TS happy. Now TS is happy when I'm just passing this to the function call.

[jiayihu]

Since serializedProps is an interface, i can still use the merge property of interfaces and make that work, correct? i just need to do it outside the class declaration.

Yes exactly. Note: declaration merging is done on module level.

// fabric.d.ts

import { SerializedRectProps } from "fabric"

declare module "fabric" {
  interface SerializedRectProps {
    name: 'string';
  }
}

The only difference is that i would act on all the the Rect classes since the interface merge i think is global?

Yes correct, that's indeed a drawback. I think that if we make toObject(): SProps return SProps from the class generic, without making it a type parameter of toObject, it could still work while allowing to define different classes of Rect with different serializable props. I believe that in that case TS would still be happy because it would see class Square extends Rect<SerializedSquareProps> so there would be a direct relationship between toObject(): SerializedSquareProps and toObject(): SerializedRectProps.

You still can't do it for a single rect object on-the-fly as in your example, but it could be a good compromise.

[ShaMan123]

I need to read this again and think about it

[asturur]

making interface changes at class level is not big issue.
Interface changes is an alternative to subclassing for simple needs, and comes with constrains. So that is a no-concern for me.

I also need to read this deeply, i m ok with a bit of simplification at the cost of neglegible drawbacks

[ShaMan123]

Guys I was thinking
Do we really need this signature in v6?
IMO we can remove this concept entirely and let the dev worry about it by subclassing or prototype mutation

- toObject(propsToInclude) {
+ toObject() {

Fabric barely uses it

[jiayihu]

I'd be fine indeed with simplifying the signature as it simplifies the typing, giving also more freedom to the caller. Yesterday I found another case of this pattern:

getObjects(...types: string[]): FabricObject[]

This forces to do getObjects("myObjectType") as MyObjectType[] but it's dangerous as getObjects("anotherType") as MyObjectType[] works as well. We could make getObjects more generic but the simplest solution is to just leave the filtering to the caller:

const isMyObject = (o: FabricObject): o is MyObjectType => o.type === "myObjectType"

const myObjects: MyObjectType[] = canvas.getObjects().filter(isMyObject) // Okay

This perfect:

1. It simplifies the fabric implementation
2. Gives more freedom to the user
3. No performance downside

So I think a similar pattern could be followed here for toObject()

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs 😔. Thank you for your contributions.

[workerbee223]

npm i --save-dev @types/fabric : EDIT @types/fabric is an unsupported external package that is not compatible with fabric6

This solved my problem when encountering this.