TS(StackedObject): refactor method types

[jiayihu]

I decided to give StackedObjects method return types a try because they are awful to work with. You basically always have to use type assertion or widen to any. The implementation itself is full of unsafe type assertions or usage of any.

• getAncestors(true) can only return Group[] in strict mode, so you should be able to just do const parents: fabric.Group[] = obj.getAncestors(true). That's not possible currently.
• getAncestors(false) returns a weird tuple that makes TS crazy. You should instead be able to refine it simply using getAncestors(false).filter(isGroup) or getAncestors(false).filter(isCanvas)
• findCommonAncestors is horrible, the return type is basically unusable because it mixes StackedObject, Group, Canvas, StaticCanvas

So the current types make the code unsafe for consumers but also for fabric's own implementation. There are several places where the risk of a bug is not evident but hiding there:

getAncestors

ancestors has type TAncestor, which allows for StackedObject but actually in this case only groups or canvas can be returned

findCommonAncestors

The whole implementation is a mess. You basically can't even tell if a value is a StackedObject, Group or Canvas. Indeed TS complains a lot here. After I added some artificial isGroup type narrowing TS is a bit happier but I don't even know if that's okay, because in the tests it's possible to do object.findCommonAncestors(canvas) but the type of the parameter doesn't allow Canvas.

Conclusion

These methods are simple calculations so it should not be hard to type them in TS. This is a code small and sign that the signatures are bad. They confuse the type checker as much as the human developer. For instance I was thinking of alternatives:

• Return only Group[] from getAncestors() and remove the strict param. If you want to check if two objects belong to the same canvas, do it manually on the caller. E.g. const ancestors = [...obj.getAncestors(), obj.canvas].filter(x => !!x). In my experience I rarely need the canvas to be accounted for and in that case it can easily be done on the caller.
• Similarly findCommonAncestors returns strictly only { fork: Group[], otherFork: Group[], common: Group[] } as well. This means that
  • It should not check for canvas. Leave it to the caller
  • StackedObject cannot be returned in any result. I don't see why we do it, it makes the types so more complicated and I doesn't make sense logically. "common ancestors" should be only "ancestors", thus Groups. Instead we have this weird case where if the two objs are the same, common starts with this so it can indeed be a simple StackedObject as well.

I think many of the weird choices in findCommonAncestors were made as convenience for isInFrontOf, the only use case in fabric. But it's making the life hard for any actual usage outside of fabric. Since these methods were added in v6, I don't think we should release such a bad API to stable.

Tldr: moving the responsibility to the caller greatly simplifies getAncestors implementation and types, especially for the common uses where you expect only parent groups.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[asturur]

i won't be able to get here before monday! I will be travelling the weekend, i ll give it a look as soon as possible

[asturur]

what the slices are doing here? why slice + spread?

[jiayihu]

To make TS happy because the slice return type will return Ancestors<S>

[asturur]

I understand that this line, and line 131 and line 139 are adding the check isGroup(x) but in all three cases the group condition is checked implcitly by the statement that follows.
if this === otherAncestors[otherAncestors.length - 1] then this must be a group, and if ancestor === other then other must be a group and etc etc, but i wish those details are called out in the pr description rather than to the reader to understand why there are code changes in a type changes pr.

[jiayihu]

It was added to make TS happy as otherwise it thinks you're comparing an object that is potentially not a group to an ancestor. I had pointed it out in the PR description:

After I added some artificial isGroup type narrowing TS is a bit happier

It was one of the main points of the PR description, i.e. the implementation requires hacking around TS because of the mixing of StackedObject, Group, Canvas, StaticCanvas

[asturur]

i read that, i was asking if you think the ancestor could be a non group. i was arguing that is always a group.

[jiayihu]

An ancestor can be a non-group as it can be a canvas or Group, but this must be an object, thus it must be a Group as well. Basically checking this against an ancestor must mean both sides are groups.

The only exception is other, which in the tests can be a Canvas, but it cannot be in the method types (both before and after this PR). But I think that's just a mistake, comparing an object against a canvas doesn't make any sense.

So I'd argue it's always a group as well.

[asturur]

@jiayihu in general saying this code is bad, this implementation is horrible or a mess, doesn't help ever.
Who sees that is a complex piece of code already knows that, the others that do not see it would not get it with some adjective added like that and may take it as an offense. In general to foster a space where everyone is happy to write or comment code i try to avoid that.

The whole stackedObject code was written to support more complicated situations for isInFrontOf to support some niche use case of activeSelection clicking across nested groups. I can't remember anything else about it. At some point it wasn't possible to just compare the indexOf and this code was written.

There is the strict / non strict part that in my opinion adds confusion for little gain. I m not sure what is supposed to do including the common ancestor up to the canvas or not including it. Every situation in which you manage to put a canvas inside another canvas and make it work is not meant to be supported. So if you are comparing object from canvas A to canvas B and you want to know which one is in front of the other you are doing something that is not supported and you get a result that may be not correct.

I would suggest at this point the original strict/ non strict PR gets recovered and comments get read to understand why strict was added, and then discussed to remove it and then look again at types, at least things are done with some history context.

[asturur]

I m not even sure why TAncestor needs to include StackedObject since all the Ancestors are either Groups or Canvases, so not sure why an Ancestor may ever be a simple object like a Rect.

[asturur]

The ancestry with strict / non strict has been introduced as part of the group rewrite, but there are no notes in the PR that let me understand why was it necessary.

[jiayihu]

Maybe @ShaMan123 can give us more context for the initial requirements when he's back

[asturur]

@jiayihu i won't be able to correctly address the conflicts here.