Replace BaseFabricObject with FabricObject

[jiayihu]

Collection is a mixin, which means it is created statically.
Class generics are dynamic, bound to the instance.
Meaning that TS doesn't allow us to type a mixin with generics.
Hence, we can't pass a generic type (e.g. the object type) to the collection w/o a lot of ugly workarounds.

Since using BaseFabricObject is not possible for now we have removed it from Collection and other types to make our lives easier.

[jiayihu]

Added minor fix so that canvas.setHeight(window.innerHeight) works, instead of requiring canvas.setHeight(window.innerHeight, {})

[ShaMan123]

I wanted to fix this, but it should be made into a generic (one day at least)
For now it is good enough since there is no option to use the base object

[asturur]

This is ok till we don't have better code splitting and tree shaking, but i agree it should be a generic Collectio<BaseFabricOject | FabricObject>

[asturur]

Yet another PR to fix TS issues. In this case, the error is simply:

const o: fabric.Object[] = new fabric.Group().getObjects()

That's because getObjects() returns BaseFabricObject[], so it's not assignable to fabric.Object[] because fabric.Object extends BaseFabricObject. It's unsafe to assign a base type to a subtype. I don't know however why BaseFabricObject was used instead of FabricObject.

To answer the question why: we want that who uses a static canvas only doesn't have to import all of fabric. StaticCanvas + non interactive objects is like half of the library.

[asturur]

You need to add a changelog line.

[ShaMan123]

We can try that now before merging

[ShaMan123]

I have tried making it generic but we are facing yet another TS limitation.
Since creating a mixin is static and generics are tied to the instance there is no way to inform the collection of the type of the object from the instance.
The only workaround I can think of, which is so ugly I am ashamed to write it down, it to make each method of collection generic. But I won't approve that.
I tried with declaration merging... That doesn't seem to work as well. You can see in the first image TS recognizes the generic as Circle but is unable to infer the Collection type.

[ShaMan123]

@asturur a note not for now:
if we make collection a standalone manager this will go away because we will init it in group/canvas constructor so they share the same generic of the object type.
Then we expose of the same methods on group/canvas and one day we deprecate

[jiayihu]

we want that who uses a static canvas only doesn't have to import all of fabric. StaticCanvas + non interactive objects is like half of the library.

That's a good use case, but yeah I don't have any idea other than maybe creating artificial different classes like:

class Collection<T extends BaseFabricObject> {
  getObjects(): T[];
}

class BaseGroup<T extends BaseFabricObject> extends Collection<T> {}

// the common interactive one
class Group extends BaseGroup<FabricObject> {}

[ShaMan123]

we want that who uses a static canvas only doesn't have to import all of fabric. StaticCanvas + non interactive objects is like half of the library.

That's a good use case, but yeah I don't have any idea other than maybe creating artificial different classes like:

class Collection<T extends BaseFabricObject> {
  getObjects(): T[];
}

class BaseGroup<T extends BaseFabricObject> extends Collection<T> {}

// the common interactive one
class Group extends BaseGroup<FabricObject> {}

Since I know you are for less subclassing this is not good enough for you, nor for fabric.
Then you want to impl some logic on group and you must use mixins again to apply the logic to both classes - messy.
Then you might want a flag that toggles the interaction state of the group/objects so this will force you to create new objects and remove the old. True, this is not the "right" way to do but still it is another point against this concept.

As I see it, this is no working around the fact that TS is not providing us what we need here.

[jiayihu]

Since I know you are for less subclassing this is not good enough for you, nor for fabric.

I haven't tried but maybe const Group = BaseGroup<FabricObject> works as well?

Then you might want a flag that toggles the interaction state of the group/objects so this will force you to create new objects and remove the old.

If I understand correctly, in that case you should be fine having the type FabricObject if the interactivity is dynamic with a toggle. For instance tree shaking would also not used in that case.

As I see it, this is no working around the fact that TS is not providing us what we need here.

As discussed together, it's definitely a tough spot the combo static analysis and dynamic behaviours.

Anyway, if there are no alternatives, are you fine with this PR that at least makes TS work in the most common case of interactive objects?

[ShaMan123]

Yes sure
Better this way
Then one day when fabric is in packages we tackle this

[ShaMan123]

@asturur any objections?

[ShaMan123]

I have fixed onDeselect since it is an interactive object always.
Updated collection events to reflect the changes made in the PR.
And tweaked TToCanvasElementOptions for future use

Other usages that seem fine as is but might cause trouble as well:

• src/util/misc/objectTransforms.ts
• src/util/typeAssertions.ts
• sendObjectToPlane

[ShaMan123]

used only by onDeselect

[ShaMan123]

I don't like this actually

[ShaMan123]

but it will be solved when we manage to type Collection