chore(TS) Ts convert event mixins #8519

asturur · 2022-12-18T13:52:11Z

Motivation

Continuing the TS migration.

Description

Lots of things going on here.

migrated the grouping mixin inside the events mixin since it does not make sense alone.
This mixin ended up extending the Canvas and so i flipped the role of the class,i intend to rollback that but not now.
Event typing are hard. Looks like we fire events with a different kind of options depending on the situation and i had to make many things optional
Going back and forth with optional properties being undefined or null.

Changes

swapped the order of arguments of very internal method for consistency
merged some internal methods into a single one

Gist

In Action

github-actions · 2022-12-25T23:43:28Z

Build Stats

file / KB (diff)	bundled	minified
fabric	958.979 (-4.078)	306.758 (-1.069)

asturur · 2022-12-26T00:12:32Z

17k less i broke something large

asturur · 2022-12-26T00:17:07Z

Not really ready for review, but we can start to have a look

asturur · 2022-12-26T00:18:53Z

only 12 UTs broken, seems very little.

…js into ts-convert-event-mixins

github-actions · 2022-12-26T10:59:18Z

Coverage after merging ts-convert-event-mixins into master will be

82.84%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
HEADER.js	54%	48.15%	0%	63.64%	12, 14, 14, 14, 14, 14, 16–17, 21, 21–22, 22, 22, 24, 24, 26, 28, 30–31, 77, 77, 77
src
cache.ts	97.06%	90%	100%	100%	56
config.ts	77.27%	66.67%	66.67%	84.62%	130, 138–140, 151–153
constants.ts	100%	100%	100%	100%
intersection.class.ts	100%	100%	100%	100%
pattern.class.ts	92.31%	85.71%	100%	96.43%	120, 126, 137, 146, 98
point.class.ts	100%	100%	100%	100%
shadow.class.ts	98.51%	96%	100%	100%	157
src/brushes
base_brush.class.ts	100%	100%	100%	100%
circle_brush.class.ts	1.52%	0%	0%	2%	101, 103–105, 114, 114, 114, 116, 118, 120–122, 124–127, 135, 142, 144, 24, 29–30, 38–42, 46–50, 57–60, 68–72, 74, 82, 82, 82, 82, 82–83, 85, 85, 85–88, 90, 98–99
pattern_brush.class.ts	97.14%	87.50%	100%	100%	22
pencil_brush.class.ts	91.91%	85.42%	100%	93.64%	123–124, 153, 153–155, 277, 281, 286–287, 69–70, 85–86
spray_brush.class.ts	1.16%	0%	0%	1.56%	100–102, 104–105, 113, 113, 113, 113, 113–114, 116–117, 124–125, 127, 129–133, 142, 146–147, 147, 155, 155, 155–158, 160–163, 167–168, 170, 172–175, 178, 185–186, 188, 190–191, 193, 200–201, 203–204, 207, 207, 214, 214, 218, 23–24, 26–28, 28, 28–30, 34, 43, 50, 57, 64, 71, 78, 90–92
src/canvas
canvas.class.ts	92.51%	88.15%	94.12%	95.44%	1150, 1150–1151, 1154, 1174, 1174, 1236, 1272–1273, 1292–1293, 1301–1302, 1323, 1331, 1444–1445, 1447–1448, 1468–1469, 1631–1632, 1636, 509, 576–577, 582, 592, 721–722, 724–725, 725, 725, 771–772, 833–834, 837, 839, 884–886, 916, 921–922, 951–952
canvas_events.ts	77.98%	75%	82.54%	79.36%	1002–1003, 1003, 1003–1005, 1007–1008, 1008, 1008, 1010, 1018, 1018, 1018–1020, 1020, 1020, 1026–1027, 1035–1036, 1036, 1036–1037, 1042, 1044, 1074–1076, 1079–1080, 1084–1085, 1183, 1203–1205, 1208–1209, 1281, 1368, 1401, 144, 1495–1496, 1502, 1506–1507, 1523, 1545, 1592, 1597, 1636, 169, 317–318, 318, 318–319, 319, 322–326, 331, 333, 345–347, 350, 367, 367, 367–368, 368, 368–369, 377, 382–383, 383, 383–384, 386, 395, 401–402, 402, 402, 438, 442, 442, 442, 442, 442–443, 445, 520, 522, 522, 522–524, 544, 546, 546, 546–547, 547, 547, 550, 550, 550–551, 554, 563–564, 566–567, 569, 569–570, 572–573, 585–586, 586, 586–587, 589–593, 599, 606, 646, 646, 646, 648, 650–654, 660, 666, 666, 666–667, 669, 672, 677, 690, 717, 773–774, 774, 774–775, 777, 780–781, 781, 781–782, 784–785, 788, 788–790, 793–794, 804, 886, 900, 907, 928, 960, 981–982, 988
canvas_gestures.mixin.ts	9.21%	5.56%	7.14%	11.36%	101, 114, 127, 139, 148, 157–161, 170, 172, 172, 172–173, 175–176, 189, 198, 207, 211, 30, 30, 30–31, 31, 31, 31, 36, 39–40, 40, 40–41, 47, 50, 58, 58, 58, 58, 58–59, 62–64, 66–67, 69, 71, 71, 71–73, 76, 78, 88
static_canvas.class.ts	92.38%	85.82%	97.92%	95.16%	1118–1119, 1119, 1119–1120, 1240, 1250, 1304–1305, 1308, 1343–1344, 1422, 1431, 1436, 1551, 1553, 1555, 1555, 1555, 1555, 1558, 1558, 1558–1559, 1611–1613, 1615, 1617, 1617, 1617, 1617, 1621, 1621, 1621–1623, 1694, 1694–1695, 1750, 1752–1753, 1753, 1753, 1756–1757, 1757, 1757, 301, 333, 346, 402–403, 405–406, 415, 419–420, 422–423, 878
src/color
color.class.ts	91.67%	84.51%	100%	94.44%	325–326, 330–331, 334–335, 41, 45, 72–73, 73, 75, 75, 75–76, 78–79
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
index.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
actions.ts	100%	100%	100%	100%
changeWidth.ts	100%	100%	100%	100%
control.class.ts	93.98%	88.89%	90.91%	97.78%	236, 320, 320, 355
controls.render.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
default_controls.ts	86.67%	66.67%	100%	100%	125, 132
drag.ts	100%	100%	100%	100%
rotate.ts	20%	12.50%	50%	22.22%	45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81

ShaMan123

fixed types by adding pointer, absolutePointer, isClick to some events. Hope I did it right.
Made me think that it is dangerous sending the cached points down to a subscriber. What if they are mutated down there?
The canvas_events diff is impossible so I can only read it.
I suggest extracting all draggable logic to a subclass one day

Now of course tests fail

ShaMan123 · 2022-12-28T14:13:49Z

src/canvas/canvas_gestures.mixin.ts

@@ -154,12 +173,39 @@ import { scalingEqually } from '../controls/actions';
          return;
        }
        t.target.rotate(radiansToDegrees(degreesToRadians(curAngle) + t.theta));


what is this?

why not ?

Suggested change

t.target.rotate(radiansToDegrees(degreesToRadians(curAngle) + t.theta));

t.target.rotate(curAngle + radiansToDegrees(t.theta)));

ShaMan123 · 2022-12-28T14:25:40Z

src/canvas/canvas.class.ts

-    }
-    this._currentTransform = transform;
-    // @ts-ignore
-    this._beforeTransform(e);


moved because of this

reverted, should be moved one day

i tried this as well, and there were too many moving parts. The real issue is that all this stuff is highly coupled together and the canvas without events would be a very tiny layer that doesn't make much sense. On the other hand canvas + canvs_events would be a 3000 line file

yeah
maybe we return to this and reshape it

src/shapes/Object/InteractiveObject.ts

src/canvas/canvas_events.ts

ShaMan123 · 2022-12-28T14:58:12Z

src/canvas/canvas_events.ts

+        '_onDragLeave',
+        '_onDrop',
+      ] as const
+    ).forEach((eventHandler) => {


IMO this method should die in favor of the fat arrow syntax that binds the methods in class init

class Canvas { boundForGood = () => { this // is always a canvas instance } }

I think this an inversion of cause and effect and i don't want to do it.
Fat arrow do not have a THIS and so in order to be used in a class they get bound at constructor time, otherwise when you call class.method() you have no idea which the THIS is if you don't bind it.

So what happens is that you need to bind them to even work in a class, and so as a side effect, being bound, they work as event handlers.

Once those are written down as fat arrow, in 3 months someone wants to convert them to function, there is no written anywhere that those should be bound again.

In our case those methods are supposed to be event handlers, so they need to be bound to the class because they will be executed out of context, and so we bind them explicitly.

MDN is clear on the fact that arrow functions are not good for classes: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions and the short hand syntax indeed make them not even more concise.

Those are useful exactly when you need an outside this for your expression ( forEach loops, map loops and so on ).

It says exactly what I wrote. It creates auto bound methods

ShaMan123 · 2022-12-28T14:58:50Z

src/canvas/canvas_events.ts

+   * @param {Event} [e] Event object fired on wheel event
+   */
+  private _onMouseWheel(e: MouseEvent) {
+    this.__onMouseWheel(e);


why is this wrapper needed?

it isn't but i think it was done for similarities with other events

ShaMan123

I moved back _setupCurrentTransform, nearly went mad because of it

ShaMan123 · 2022-12-28T16:09:34Z

Maybe it is best I squash my commits and force push
that way you can view them properly

asturur · 2022-12-28T18:35:24Z

Maybe it is best I squash my commits and force push that way you can view them properly

Yes would be great if you can have a commit only i can read, without updating to latest master.

asturur · 2022-12-29T01:03:57Z

fixed types by adding pointer, absolutePointer, isClick to some events. Hope I did it right.

Adding more data to events to satisfy generic types wasn't my preferred solution, but also is not performance relevant so i don't care.
Event types are overly complicated to me anyway.

Made me think that it is dangerous sending the cached points down to a subscriber. What if they are mutated down there?

We could generate new, i don't think should be a big deal, or we could find a way to make the cached pointer immutable. ( Object freeze or something like that )

The canvas_events diff is impossible so I can only read it.

I suggest extracting all draggable logic to a subclass one day

Now of course tests fail

asturur · 2022-12-29T01:08:17Z

Ok i see canvas_events become a new file.
2b77ecb...e212f16
Here you should be able to read the correct diff.

ShaMan123 · 2022-12-29T05:59:02Z

Ok i see canvas_events become a new file.
2b77ecb...e212f16
Here you should be able to read the correct diff.

Couldn't, even when moving the file back. Too large a change.
That is that

ShaMan123

@asturur take a look
for future large PRs I think we should try to keep history by renaming first and then changing files. But no sure that would have worked in this case since changes were too big.

ShaMan123 · 2023-01-03T07:00:00Z

src/canvas/canvas.class.ts

@@ -833,10 +836,7 @@ export class Canvas<
    let pointer = this.getPointer(e);
    if (target.group) {
      // transform pointer to target's containing coordinate plane
-      // should we use send point to plane?
-      pointer = pointer.transform(
-        invertTransform(target.group.calcTransformMatrix())


this was wrongly migrated

ShaMan123

this is the error to be fixed by #8563

ShaMan123 · 2023-01-03T08:00:27Z

src/canvas/canvas.class.ts

-      pointer = pointer.transform(
-        invertTransform(target.group.calcTransformMatrix())
-      );
+      pointer = sendPointToPlane(pointer, target.group.calcTransformMatrix());


should be sendPointToPlane(pointer, undefined, target.group.calcTransformMatrix());

ShaMan123 · 2023-01-03T08:08:06Z

I got extremely confused
_setupCurrentTransform
_transformObject
both need to act on the pointer in the same way

Co-authored-by: ShaMan123 <[email protected]>

asturur added 11 commits December 12, 2022 01:12

broken progress

7c83395

save work

fdf9245

save progress work

aa0b140

may i have broke stuff

6c02e74

what did i do

84541aa

something killed fabric.config

135ef2a

stupid vscode you killed a full evening

54e6351

test passing

5ba11af

minimal changes

47d717b

clean up canvas

305d6e8

save work

cbfd44f

asturur mentioned this pull request Dec 18, 2022

chore(TS): Convert Canvas to TS #8510

Merged

minor adjustments

b9d98fa

ShaMan123 mentioned this pull request Dec 18, 2022

patch(TS): canvas patch init 2 #8520

Merged

asturur added 4 commits December 18, 2022 22:11

Merge branch 'master' into ts-convert-event-mixins

368e129

converted more

076bffa

at least prettier

b21c28e

trying to cleanup

6acf71a

it builds at least

71d5dde

asturur changed the title ~~DRAFT: Ts convert event mixins - WIP~~ chore(TS) Ts convert event mixins Dec 26, 2022

tweak index

db358ee

asturur marked this pull request as ready for review December 26, 2022 00:16

asturur added 4 commits December 26, 2022 01:19

prettier

cfa48e1

add changelog

cee73f6

Merge branch 'master' into ts-convert-event-mixins

303a897

bugs removed

8bd2b17

asturur added 3 commits December 26, 2022 11:37

Merge branch 'ts-convert-event-mixins' of github.com:fabricjs/fabric.…

af6f4ad

…js into ts-convert-event-mixins

correct arg order

a5ee9f8

fixed more tests

983f162

asturur added 3 commits December 26, 2022 12:13

prettier

f1ea8e4

save - work

e212f16

moved fies in canvas folder

d7e4128

ShaMan123 reviewed Dec 28, 2022

View reviewed changes

ShaMan123 previously approved these changes Dec 28, 2022

View reviewed changes

This is a combination of 15 commits.

f6540c9

asturur dismissed ShaMan123’s stale review via f6540c9 December 29, 2022 00:44

asturur force-pushed the ts-convert-event-mixins branch from 9dc4797 to f6540c9 Compare December 29, 2022 00:44

asturur added 2 commits December 29, 2022 01:51

revert unnecessary import changes

dcf31b7

Merge branch 'master' into ts-convert-event-mixins

32c0531

asturur merged commit b6f076c into master Dec 29, 2022

asturur mentioned this pull request Dec 29, 2022

🧐 Breaking changes for fabric 6.0 #8299

Open

ShaMan123 reviewed Jan 3, 2023

View reviewed changes

ShaMan123 mentioned this pull request Jan 3, 2023

fix(): migration regression #8563

Merged

ShaMan123 reviewed Jan 3, 2023

View reviewed changes

frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023

chore(TS) Ts convert event mixins (fabricjs#8519)

54767de

Co-authored-by: ShaMan123 <[email protected]>

ShaMan123 deleted the ts-convert-event-mixins branch February 5, 2023 05:37

ShaMan123 added a commit that referenced this pull request Mar 16, 2024

chore(TS) Ts convert event mixins (#8519)

7f3ec26

Co-authored-by: ShaMan123 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TS) Ts convert event mixins #8519

chore(TS) Ts convert event mixins #8519

asturur commented Dec 18, 2022 •

edited

Loading

github-actions bot commented Dec 25, 2022 •

edited

Loading

asturur commented Dec 26, 2022

asturur commented Dec 26, 2022

asturur commented Dec 26, 2022

github-actions bot commented Dec 26, 2022 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Dec 28, 2022

ShaMan123 Dec 28, 2022 •

edited

Loading

ShaMan123 Dec 28, 2022

ShaMan123 Dec 28, 2022 •

edited

Loading

asturur Dec 28, 2022 •

edited

Loading

ShaMan123 Dec 29, 2022

ShaMan123 Dec 28, 2022 •

edited

Loading

asturur Dec 28, 2022

ShaMan123 Dec 29, 2022

ShaMan123 Dec 28, 2022

asturur Dec 28, 2022

ShaMan123 left a comment

ShaMan123 commented Dec 28, 2022

asturur commented Dec 28, 2022

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022

ShaMan123 commented Dec 29, 2022

ShaMan123 left a comment

ShaMan123 Jan 3, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 Jan 3, 2023

ShaMan123 Jan 3, 2023

ShaMan123 commented Jan 3, 2023

	t.target.rotate(radiansToDegrees(degreesToRadians(curAngle) + t.theta));
	t.target.rotate(curAngle + radiansToDegrees(t.theta)));

chore(TS) Ts convert event mixins #8519

chore(TS) Ts convert event mixins #8519

Conversation

asturur commented Dec 18, 2022 • edited Loading

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Dec 25, 2022 • edited Loading

asturur commented Dec 26, 2022

asturur commented Dec 26, 2022

asturur commented Dec 26, 2022

github-actions bot commented Dec 26, 2022 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

asturur Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 commented Dec 28, 2022

asturur commented Dec 28, 2022

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022

ShaMan123 commented Dec 29, 2022

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Jan 3, 2023

asturur commented Dec 18, 2022 •

edited

Loading

github-actions bot commented Dec 25, 2022 •

edited

Loading

github-actions bot commented Dec 26, 2022 •

edited

Loading

ShaMan123 Dec 28, 2022 •

edited

Loading

ShaMan123 Dec 28, 2022 •

edited

Loading

asturur Dec 28, 2022 •

edited

Loading

ShaMan123 Dec 28, 2022 •

edited

Loading

ShaMan123 Jan 3, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading