chore(TS) Converting Object to a class #8322

asturur · 2022-09-27T10:41:52Z

This PR propose a way to convert fabric obects to classes while maintaining flexibility on the prototype.
This PR also includes the work from @ShaMan123 around converting the observable in a class and is merged using his own commits.

TLDR;

The class is declared as an es6 class, but the fields are just declared as types.

A separate object contains all the class default values and those get added on the prototype with an assignment.

The class is fully typed, and the default values are type checked using an utilty type function.

Changes

You should skip all files but object.class.js, observable.class.js, and typdefs.ts

The changes in object.class.ts are unreadable, but what happened is this:

The class declaration is:

class FabricObject {
  let: number;
  top: number;
  constructor(options) {
    this.setOptions(options);
  }
  initialize(options) {
    ... for compatibility reason, removed as soon as we changed everything to class
  }
  ... all the old methods, no properties...
}

The methods changed from:

setOptions: function() { code },

to

setOptions() { code }

Nothing else has been touched

All the properties are at the end of the file and are assigned like this:

Object.assign(
  FabricObject.prototype,
  fabric.CommonMethods, 
  fabricObjectDefaultValues,

The observable has ben converted to a class plain and simple no strange changes of any kind.

I updated all the reference not because it was necessary but because i made an error in the constructor and nothing was working, and it took me a while to see the error.

Type checking

This utility i found on stackoverflow removes from a type all parts that are functions.
This allow us to know from the class definition, by difference, which are the properties we expect in the defaul object values, and allow us to have typeChecking

// eslint-disable-next-line @typescript-eslint/ban-types
type TNonFunctionPropertyNames<T> = { [K in keyof T]: T[K] extends Function ? never : K }[keyof T];
export type TClassProperties<T> = Pick<T, TNonFunctionPropertyNames<T>>;

Todos

get agreement on this pattern -> NOT done, won't be done
resolve the type TDegree and number
add the optional typing on the class for all the items we add at runtime on the FabricObject and that are undocumented ( _cacheCanvas to make an example ).
get the object.class.ts file type check and linted -> DONE partially
remeber how to do checklists in MD on github

asturur · 2022-09-27T10:42:56Z

src/mixins/observable.mixin.ts

+  fabricHeader.Observable = {
+    fire: fire,
+    on: on,
+    once: once,
+    off: off,
+  }


this is some bullshit i did when i was half asleep.
I didn't even remember doing it.
Going to delete

I saw that in #8103 and removed that there as well

seems like i added it here?

The diff is too harsh.

asturur · 2022-09-27T11:02:59Z

src/gradient/gradient.class.ts

@@ -93,7 +90,7 @@ export class Gradient<
    gradientTransform,
    id,
  }: GradientOptions<T>) {
-    const uid = fabric.Object.__uid++;
+    const uid = FabricObject.__uid++;


this is not the place for the __uid. i remember @ShaMan123 already worked to put it somewhere else

ShaMan123

As mentioned previously cacheProperties and stateProperties should be static IMO. Is there any need for them on the proto?
All calls to FabricObject.prototype[method] should be replaced with super[method]

Where do you assign a prototype value to the class?

ShaMan123 · 2022-09-27T13:30:08Z

src/parser/setStrokeFillOpacity.ts

        continue;
      }
-      attributes[attr] = fabric.Object.prototype[attr];
+      attributes[attr] = FabricObject.prototype[attr];


I think this part can be removed in the next iteration.
Why would we need a default value if parsing will eventually init a new class that already has defaults and handles that internally.

The code speaks about opacity, i assume that if the svg specify opacity 0.5, we have to transfer that on a color, and that color if not differently specified is our default one.

Perhaps it should be exposed as a static method on FabricObject...?
Doesn't sound it will fix anything.
This is a design thing...

ShaMan123 · 2022-09-27T13:33:23Z

src/util/lang_class.ts


  if (parent) {
    Subclass.prototype = parent.prototype;
    klass.prototype = new Subclass();
-    parent.subclasses.push(klass);


I am guessing this makes a lot of tests fail
Textbox etc.

i din't see the word subclasses used in the codebase outside this line, so i tried to get it away. I ll look better

I think (not sure) that callSuper uses something of that logic

@ShaMan123 callSuper uses superclasses

So is it for extend?
I can't scan the codebase currently

i think it was unused

asturur · 2022-09-27T13:55:33Z

As mentioned previously cacheProperties and stateProperties should be static IMO. Is there any need for them on the proto? All calls to FabricObject.prototype[method] should be replaced with super[method]

Where do you assign a prototype value to the class?

Maybe and yes.
But those are not the kind of comments/changes we are looking at now.

Let's look a the general approach.

To solve the first issue, simple typings, later i should try to add here that kind of mixin function you shown me yesterday so that it can take in account also properties where possible.

cacheProperties and stateProperties are good to be accessed with this once they are accessed with this, each class can change its own per class. Otherwise you have to refer to Rect.cacheProperties and Circle.cacheProperties? how do you handle the i look at the nearest one in the prototype chain if are static fields/properties?

ShaMan123 · 2022-09-27T14:26:22Z

There is no static inheritance AFAIK.
I would argue that anything shared (such as cache/state props) should be static and anything at the instance level shouldn't be shared as a strict design rule. I understand that referencing this is more convenient but if you're using this, you've imported the class so it doesn't make that much of a difference.

asturur · 2022-09-28T21:07:02Z

the fact is that the cache properties may be definied per class, but accessed by the base class.

Co-authored-by: ShaMan123 <shacharnen@gmail.com> Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Lazauya · 2022-10-12T23:14:12Z

src/shapes/object.class.ts

+ * @fires drop
+ */
+
+const fabricObjectDefaultValues = {


maybe give this object a type? like const fabricObjectDefaultValues: FabricObject or const fabricObjectDefaultValues: Partial<FabricObject>

Yes, that is where i m stuck now.
Type verification of this object.
What i did was copy pasting a utitlity from stack overflow that is this one:

f05ee9c#diff-a2ca4295c4b9b0a677c07db27810483a1abf0d671387d14ab4258f6bdfd72cfaR21-R22

And i m trying to extract the types from the class declaration and enforce the default values to respect that.
But isn't really working for now.

oh well :)

it was working, is just that i put back the ts-nocheck comment to avoid too many errors.

asturur · 2022-10-13T13:33:51Z

no assigning them in the constructor would make every instance have a copy of the default value as an own property.
is exactly what the public field do and what i want to avoid

This is an anti pattern, is it not? What are the advantages? Why do you want/need it?

Again i want to be able, as today, to change default values at runtime.
assigning all defaults in the constructor ( aka public fields ) breaks it.

ShaMan123 · 2022-10-13T13:36:26Z

_set is another anti pattern since it runs in the constructor and executes side effects so if you subclass _set will run in super() calls and try to execute side effects on uninitialized class properties

ShaMan123 · 2022-10-13T13:39:00Z

Again i want to be able, as today, to change default values at runtime.
assigning all defaults in the constructor ( aka public fields ) breaks it.

Finally I get it.
An anti pattern IMO.
We discuss it tomorrow.

ShaMan123 · 2022-10-13T13:42:58Z

You are talking about reactivity. Like mobx. Fabric should not do that.

ShaMan123 · 2022-10-13T13:54:26Z

In other words, to make myself clear, a shifting/shared class state is an anti pattern

asturur · 2022-10-13T14:28:36Z

Fabric won't use it in the codebase.
The shared state is still there, the class prototype with the functions to which you can do what you want, with or without our change.
Allowing it also for default values on top of functions is just less confusing

asturur · 2022-10-13T14:32:29Z

_set is another anti pattern since it runs in the constructor and executes side effects so if you subclass _set will run in super() calls and try to execute side effects on uninitialized class properties

but set is not giving any functionality that can't be obtained with similar code.
And that can be done without having all developers need to adapt.

Also the fact that yes, properties needs to be initialized and do not exist when you initialize the class in the super(), is very annoying and another point on the side of not using public fields and constructor initialization.

ShaMan123 · 2022-10-13T14:52:31Z

I will reply in our chat!

asturur · 2022-10-17T10:20:20Z

UPDATE

After some chit chat around good patterns and not good patterns, while perfectly in the realm of opinions, we decided to give a go to @ShaMan123 idea of not adding items to the prototype.

This won't be done in this PR.

The list of work items is:

have this PR converting object into class with TS complaining not that much ( but complaining )
have a rendevouz over object geometry mixin and move there everything that is a dependency so that it can be its own class, have object extend the geometry part ( so that the common base for any object is Geometry + observable + common methods and that is not configurable )
move away from object SVG specific methods ( just 2 of them in the svg parser logic )
convert the other shapes to classes in this way ( are all easy, while Text is another big chunk of work )

then have a PR ( or more than one ) that move the defaults out of the prototype so that breaking changes and diffs are captured and isolated there.

asturur · 2022-10-17T10:23:08Z

src/shapes/object.class.ts

+   * @default undefined
+   * @private
+   */
+  _cacheContext: CanvasRenderingContext2D | null = null;


this is a public field, this thing is either null or not and does not need to be configured ever

Is public for what reason?
Context is needed

i mean is not public, it can be private. But it does need to be in the prototype ever

asturur · 2022-10-17T10:23:47Z

src/shapes/object.class.ts

+   * @default undefined
+   * @private
+   */
+  canvas?: StaticCanvas | Canvas;


all the types below this one are new and added reading the code of object.class

…ic.js into converting-object-to-class

github-actions · 2022-10-17T13:00:16Z

Coverage after merging converting-object-to-class into master will be

82.35%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
HEADER.js	54.90%	48.15%	0%	65.22%	105, 105, 105, 12, 14, 14, 14, 14, 14, 16–17, 21, 21–22, 22, 22, 24, 24, 26, 28, 30–31
src
cache.ts	97.06%	90%	100%	100%	57
canvas.class.ts	93.44%	90.36%	94.23%	95.58%	1078, 1078–1079, 1082, 1102, 1102, 1137, 1170–1171, 1199–1200, 1233, 1241, 1351–1352, 1354–1355, 1375–1376, 1534, 1539, 1549, 1553, 486–487, 492, 501, 650–652, 697–698, 762–763, 766, 768, 815–817, 859, 864–865, 893–894
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.19%	85.71%	100%	96.30%	110, 116, 127, 136, 88
point.class.ts	100%	100%	100%	100%
shadow.class.ts	95.95%	90%	100%	100%	173, 240, 9
static_canvas.class.ts	90.21%	83.99%	96.77%	92.82%	1153–1154, 1154, 1154–1155, 1289, 1355–1356, 1359, 1408–1409, 1502, 1517, 1521, 1547–1548, 1577–1578, 1611–1612, 1653–1654, 1657, 1659, 1659, 1659, 1659, 1663, 1663, 1663–1665, 1687–1688, 1729–1730, 1733, 1735, 1735, 1735, 1735, 1739, 1739, 1739–1741, 1814, 1814–1815, 1888, 1890, 1890, 1890, 1890, 1890–1891, 1894–1895, 1895, 1895–1896, 1899, 1899, 1899, 1901, 1904, 1910, 1912–1913, 1913, 1913, 1916–1917, 1917, 1917, 1920, 279–280, 282–283, 285–286, 299–300, 302–303, 617, 643, 699–702, 902
src/brushes
base_brush.class.ts	100%	100%	100%	100%
circle_brush.class.ts	2.99%	0%	0%	3.92%	100–101, 103, 105–107, 116, 116, 116, 118, 120, 122–124, 126–129, 137, 144, 146, 26, 31–32, 40–44, 48–52, 59–62, 70–74, 76, 84, 84, 84, 84, 84–85, 87, 87, 87–90, 92
pattern_brush.class.ts	5.26%	0%	0%	8.33%	16, 20–23, 25–26, 26, 26–29, 37–38, 40, 44, 55, 55, 55, 63–65, 65, 65, 72–73, 75–76, 76, 80
pencil_brush.class.ts	91.95%	85.42%	100%	93.69%	125–126, 155, 155–157, 279, 283, 288–289, 71–72, 87–88
spray_brush.class.ts	2.30%	0%	0%	3.08%	102–104, 106–107, 115, 115, 115, 115, 115–116, 118–119, 126–127, 129, 131–135, 144, 148–149, 149, 157, 157, 157–160, 162–165, 169–170, 172, 174–177, 180, 187–188, 190, 192–193, 195, 202–203, 205–206, 209, 209, 216, 216, 220, 25–26, 28–30, 30, 30–32, 36, 45, 52, 59, 66, 73, 80, 92–94
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
control.class.ts	90.24%	81.48%	86.67%	97.50%	210, 304, 304, 348, 372, 6
controls.actions.ts	76.80%	69.20%	93.75%	80.67%	161, 163, 163, 163, 165, 167, 26, 28, 28, 28, 290–293, 321, 323, 330–331, 333, 333–334, 336–337, 341, 373, 375, 382–383, 385, 385–386, 388–389, 393, 421–422, 424, 426, 431, 434, 434, 434–435, 435, 435, 437, 437, 437–438, 438, 438, 441, 441, 441–442, 442, 442, 475–476, 478, 480, 485, 488, 488, 488–489, 489, 489, 491, 491, 491–492, 492, 492, 495, 495, 495–496, 496, 496, 520–522, 528, 528, 528–529, 532–535, 537, 537, 537–539, 539, 539–541, 543, 543, 543–545, 545, 545–546, 551, 551, 551–552, 554, 556–558, 57, 589–590, 592–594, 641–643, 8, 841
controls.render.ts	85.11%	84.78%	100%	84.78%	23, 27, 42–49, 58–59, 82, 86
default_controls.ts	94.29%	50%	100%	100%	117, 85
src/filters
2d_backend.class.ts	96.43%	83.33%	100%	100%	78
WebGLProbe.ts	40%	40%	57.14%	34.78%	38–39, 49–53, 61, 64, 66, 66, 66–67, 67, 67–70, 72, 74, 78
base_filter.class.ts	28.90%	31.82%	36.36%	26.17%	100–102, 102, 102–103, 112–116, 116, 116–117, 124–125, 125, 125–128, 143, 159, 169–174, 178, 181, 181, 181–184, 184, 184–185, 185, 188–189, 195, 204–205, 210–214, 257–260, 273, 273, 273–274, 276, 292–294, 294, 294, 294, 294–295, 297, 299–300, 306–307, 309–311, 315–316, 318, 322–324, 328, 332, 352, 352, 352–356, 393, 78, 78, 78–79, 79, 79–80, 80, 80–81, 86–89, 89, 89–90, 99</

…cjs/fabric.js into converting-object-to-class

ShaMan123

Looks good

_render is a good candidate to become abstract.
Shared methods have a number of functions that all come down to set. We can use signature overload. I guess you want to avoid that for now.

ShaMan123

Forgot these

ShaMan123 · 2022-10-17T17:35:10Z

src/shapes/object.class.ts

+import { createCanvasElement } from '../util/misc/dom';
+
+type StaticCanvas = any;
+type Canvas = any;


Use the Canvas type from __types__?

as soon as Canvas is converted, we can trash those all away

ShaMan123 · 2022-10-17T17:35:26Z

src/shapes/object.class.ts

+type StaticCanvas = any;
+type Canvas = any;
+
+const ALIASING_LIMIT = 2;


extract to config? or constants?

aliasing limit is not configurable, is just an extra pixel left and right to avoid cutting away a line drawn on the edge of the cache.
If we round wrong, is going to give us the missing pixel. And shouldn't be more than one pixel per side ever

@ShaMan123

* Observable changes to TS class from @ShaMan123 (fabricjs#8330) Co-authored-by: ShaMan123 <shacharnen@gmail.com> Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

asturur added 3 commits September 27, 2022 00:11

just saving

3e16059

just saving

44b96d5

ok this builds and tests

a755104

asturur commented Sep 27, 2022

View reviewed changes

asturur mentioned this pull request Sep 27, 2022

IMPORTANT: Migrating to modern javascript/typescript #7596

Closed

asturur requested review from ShaMan123 and melchiar September 27, 2022 11:00

asturur commented Sep 27, 2022

View reviewed changes

removed

cb4eb2c

ShaMan123 reviewed Sep 27, 2022

View reviewed changes

asturur and others added 12 commits September 30, 2022 01:12

test pass

2ea8540

Observable changes to TS class from @ShaMan123 (#8330)

09d32e3

Co-authored-by: ShaMan123 <shacharnen@gmail.com> Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

extends observable

fa35c31

no-need-to-expose-observable

443ced7

added observable as a class

24e02d4

prettify

0d9f0f1

ok deleted this

b77dd21

lots of typings

530775f

pretty

4fdf9e3

Merge branch 'master' into converting-object-to-class

c5a42ea

added a test

0471973

added a test

ee394dc

Lazauya reviewed Oct 12, 2022

View reviewed changes

asturur added 4 commits October 13, 2022 10:54

ok this is not working but is a start

f05ee9c

import type top

66c6ea2

this wasnt here

2e55cde

Merge branch 'master' into converting-object-to-class

b6deb8b

test passing

65e4c33

asturur force-pushed the converting-object-to-class branch from 3e93f57 to 65e4c33 Compare October 17, 2022 10:10

remove unneeded changes

7030b16

asturur commented Oct 17, 2022

View reviewed changes

asturur added 4 commits October 17, 2022 12:26

reverted accidental change

a2fa127

Merge branch 'master' into converting-object-to-class

fc8bcbb

prettier

1a426af

Merge branch 'converting-object-to-class' of github.com:fabricjs/fabr…

247f8e1

…ic.js into converting-object-to-class

Merge branch 'converting-object-to-class' of https://github.com/fabri…

cabc310

…cjs/fabric.js into converting-object-to-class

ShaMan123 previously approved these changes Oct 17, 2022

View reviewed changes

Update object.class.ts

ab2da0b

ShaMan123 dismissed their stale review via ab2da0b October 17, 2022 17:33

ShaMan123 previously approved these changes Oct 17, 2022

View reviewed changes

Merge branch 'master' into converting-object-to-class

c661f9e

asturur dismissed ShaMan123’s stale review via c661f9e October 20, 2022 09:31

asturur merged commit 8235106 into master Oct 20, 2022

ShaMan123 mentioned this pull request Oct 24, 2022

chore(TS): Observable #8103

Closed

asturur deleted the converting-object-to-class branch November 1, 2022 23:37

chore(TS) Converting Object to a class #8322

chore(TS) Converting Object to a class #8322

Conversation

asturur commented Sep 27, 2022 • edited Loading

TLDR;

Changes

Type checking

Todos

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

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

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

asturur commented Sep 27, 2022 • edited Loading

ShaMan123 commented Sep 27, 2022

asturur commented Sep 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

asturur commented Oct 13, 2022

ShaMan123 commented Oct 13, 2022

ShaMan123 commented Oct 13, 2022

ShaMan123 commented Oct 13, 2022

ShaMan123 commented Oct 13, 2022

asturur commented Oct 13, 2022

asturur commented Oct 13, 2022

ShaMan123 commented Oct 13, 2022

asturur commented Oct 17, 2022

UPDATE

Choose a reason for hiding this comment

ShaMan123 Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2022 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a 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

Choose a reason for hiding this comment

asturur commented Sep 27, 2022 •

edited

Loading

asturur commented Sep 27, 2022 •

edited

Loading

asturur commented Sep 28, 2022 •

edited

Loading

ShaMan123 Oct 17, 2022 •

edited

Loading

github-actions bot commented Oct 17, 2022 •

edited

Loading