chore(TS): export FabricObjectProps and GroupProps

[jiayihu]

More type exports to allow declaration merging.

TFabricObjectProps is also exported because it's convenient. Unfortunately it seems that when you do declaration merging, you can't import types from submodules anymore e.g. import { TFabricObjectProps } from "fabric/dist/src/shapes/Object/types". I haven't found a solution yet, so for now exporting TFabricObjectProps seems convenient since it's quite a useful type anyway.

I took the chance to change GroupEvents to interface as well, even though I don't need it. It may probably be a good practice as library author to use Interface by default and export all the interface types at the top-level to allow declaration merging. Especially for a library like fabric that relies so much on extension vs composition.

[ShaMan123]

More type exports to allow declaration merging. This one is for tests, where I'd like just to write new fabric.Object({ uuid: "123" }) or new fabric.Group({ uuid: "123" }) to test some logic that works for any fabric.Object, without having to create my own class.

This is possible now in fabric new fabric.Object<MoreProps>() but #9014 will kill this option if I am not mistaken

[jiayihu]

This is possible now in fabric new fabric.Object() but #9014 will kill this option if I am not mistaken

That PR doesn't remove Props, only SProps. Btw, I find new fabric.Object<MoreProps>() inconvenient to repeat everytime (you know how often we do it in CollaboardWeb) compared to just doing declaration merging

[ShaMan123]

Both should be respected imo
But strictly speaking this is an anti pattern for TS, isn't it?
A super class defining props for a sub class in set?

[jiayihu]

A super class defining props for a sub class in set?

I'm not following, it doesn't seem the case here.

[ShaMan123]

the prop uuid is not defined by any class, right? Strictly speaking.
It is assigned to the instance via 'set'.

[jiayihu]

Yes, but what do you mean with super class defining props for a sub class? You're defining the uuid in FabricObjectProps via declaration merging, so globally for any object, not for a subclass.

[ShaMan123]

For discussion sake:
I view you as a TS first citizen so what I mean is that you are trying to solve a problem that is caused by misuse.
You are using set which in pure TS/class rules is breaking the prototype chain, setting the uuid property.
And we are trying to make TS respect that.
The pain of declare is this exact pain.
I have no profound problem with this PR (as I said, TS should work for us IMO even if we behave badly) but regardless set is bad practice.
I thought #8980 would remedy this but it doesn't. To make that work we need to use defineProperty in the constructor (I thought TS transpiles assignment from the constructor to that automatically, but of course it does not)

My point is we are patching instead of addressing the issues TS is causing us.

[jiayihu]

I'm not following your discussion honestly.

You are using set which in pure TS/class rules is breaking the prototype chain, setting the uuid property.

I'm not using set, I'm just doing new fabric.Object({ uuid: "" }). The constructor will internally use .set but that's an implementation detail. Are you suggesting that, as consumer, creating an object with uuid is a misuse?

Just to make it clear, what's the expected code? To create a new class MockObject extends fabric.Object<PropsWithUuid> and use that class in the tests?

[ShaMan123]

I'm not using set, I'm just doing new fabric.Object({ uuid: "" }). The constructor will internally use .set but that's an implementation detail. Are you suggesting that, as consumer, creating an object with uuid is a misuse?

I am indeed, in a very strict manner of TS
This is a pattern that was brought over from past versions and has its benefits but complicates the constructor and initialization process a lot
So yes, strictly speaking, subclass.

[jiayihu]

This is a pattern that was brought over from past versions and has its benefits but complicates the constructor and initialization process a lot

Ah I see. But it's then a significant breaking change that should be document, i.e. not supporting extra properties in constructor without subclassing.

Anyway I'm fine with creating a new subclass for tests. Nervertheless exporting the types it's still useful to just use them since once you do declaration merging for anything, you can't import anymore from submodules. So I can't use const props: FabricObjectProps[] = [...], which is really useful.

[ShaMan123]

Thank you @jiayihu

[asturur]

Just to be clear about this:

you can't import types from submodules anymore e.g. import { TFabricObjectProps } from "fabric/dist/src/shapes/Object/types"

We don't have submodules, submodules aren't supported.

So we export all the types from fabric.ts, every type we need.