chore(TS): Add declare and remove useless methods from Object. #8574

asturur · 2023-01-07T09:49:47Z

Motivation

Finishing up basic object migration.

Description

change the public properties to declare so that any transpiler understand those should be removed.
~~added the default values in the constructor, while still making possible to change them globally before initialization of objects~~ failed / scope blown up
cacheProperties and stateProperties needs to stay on prototype, having them as static is not really possible since it would require duplicating them for each class and would add another constrain when subclassing. Apparently static properties aren't usable with inheritance failed / scope blown up
Fixed as much as possible TS complains, a couple have been ignored
Object._removeTransformMatrix moved as utility since is used only from the svg parser

Changes

removed Object.centerObject and similar. The methods were aliases for canvas counterparts with the added variable that they would be no-op if there wasn't a canvas. Seems jus confusing. Developers should call those methods from the canvas instance instead of relying on the fact that the canvas is maybe there maybe not and the code won't do nothing but won't fail

Gist

In Action

asturur · 2023-01-07T09:52:02Z

Messed up a bunch of things with cacheProperties i need to fix yet.
adding all the declare everywhere wil let us exports or class syntax normally.

asturur · 2023-01-07T14:50:35Z

This is going to be another painful PR on top of certifying that class were a mistake and are less powerful and useful than functions. now i m sure.

github-actions · 2023-01-07T15:38:51Z

Build Stats

file / KB (diff)	bundled	minified
fabric	928.366 (-1.966)	299.110 (-0.726)

asturur · 2023-01-07T18:39:01Z

@ShaMan123 this thing is becoming more complicated than planned.

Using public properties or defaults in the constructor is triggering the set calls. set calls executes side effects in different order depending on the order of the keys of the options and results changes.
We could update tests but it won't be stable anyway since order of iteration on keys is not guaranteed.

I already tried to assign defaults outside of set ( as we were doing before because they were on the prototype ) but that can't be done because they can't be assigned on this before the constructor run, and the constructor of the super classes expect them to be set already.

We could stop executing side effects on the constructor entirely but that would mean we would need to find what breaks and fix it manually.

asturur · 2023-01-07T18:52:28Z

Reducing the scope of the pr to unlock exports

ShaMan123 · 2023-01-08T07:07:11Z

I have to say this PR seems a step back rather than forward.
Something to discuss.
All this declare mess, did you try to alter tsconfig#strictPropertyInitialization beforehand? Is it related? Well, I guess it is easy to revert, a simple search & replace. But a lot of work for a transient step.
I still don't understand where you set the default values in the constructor. Are we still hacking it via the prototype? If so I guess that is what is causing all this mess. Am I right?
Regarding set, it is something we discussed and a fault in design as I see it. I read that class properties are initialized in the constructor with Object#defineProperty and after that via Object#set. That is the spec. And it makes sense. No side effects from the constructor block.
We should align to that. #8372 suggest a way to manage effects but I think we need a robust and simple way such as setter. I know you dislike it, I do too. You suggested proxy. I read about that. I have no idea how to incorporate with the class new command (can only think of static initializers which is a turn off).
We are at the finish line which is always the hardest part in projects, right?
We made amazing progress. Let's decide how to manage this. It is a crucial decision.
All in all my point is that we decided to align to modern js but this PR is doing the opposite.
What do you think?

ShaMan123 · 2023-01-08T07:29:44Z

take a look at this POC for circle and ellipse.
https://github.com/fabricjs/fabric.js/compare/init-define-props-poc
Circle and ellipse tests pass.

ShaMan123 · 2023-01-08T07:32:27Z

I prefer a chat if you are willing
Let's solve this and plan the end of the migration, tackling remaining concerns.

asturur · 2023-01-08T08:29:19Z

Some premise, adding items to the prototype isn't hacking.

declare is needed because otherwise those are public properties, so if you don't assign a value, you need to declare them.
The fact that the current TS plugin is fixing them ( but the new one and babel aren't ) is a hint of something wrong.

Babel transpiler either errors because there is a declare or they are actual undefined properties, but existing.

Looks like the above code is valid, so when the code gets transpiled and the type definition removed, those are undefined props, without declare ( and a removal from the transpiler ) the code is not correct because those undefined will remove values you add in constructor.

Putting the defaults in the contructor as i did in the previous commits doesn't work for many classes that have set initializators that move up and down between the sub and super classes.

Circle and ellipses works for me too, group and active selection do not.

The reason why i m not handling it in this PR anymore is because every class will need ad hoc changes to work with setting defaults in the constructor, test changes too, so we will need more PRs.

So i m descoping this PR to adding declare only, and limiting the diff to that as much as possible, so at least we can try again to enable exports from the main index.

19aa171 this is the commit where i moved all defaults in constructors and then i strated roll back.

ShaMan123 · 2023-01-08T08:38:04Z

Some premise, adding items to the prototype isn't hacking.

I thought class properties do not belong to the prototype, only to the instance. Methods belong to the prototype.

asturur · 2023-01-08T08:38:29Z

I also separated cache properties and state properties from default values because those will either need to be in some config or stay in the prototype. Their usage is unfit for static, static does not inherit with the subclasses apparently.

ShaMan123 · 2023-01-08T08:43:41Z

static field do inherit

class A {
  static method() {
    return 1;
  }
  static arr = [1,2,3]
  static type = 'A'
}

class B extends A {
  static {
    this.arr.push(4)
    this.type = 'B'
  }
  static method() {
    return super.method() + 2;
  }
}

console.log(B.method(), B.arr, B.type)

Output

"use strict";
var _a;
class A {
    static method() {
        return 1;
    }
}
A.arr = [1, 2, 3];
A.type = 'A';
class B extends A {
    static method() {
        return super.method() + 2;
    }
}
_a = B;
(() => {
    _a.arr.push(4);
    _a.type = 'B';
})();
console.log(B.method(), B.arr, B.type);

Compiler Options

{
  "compilerOptions": {
    "strict": true,
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "alwaysStrict": true,
    "esModuleInterop": true,
    "declaration": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "target": "ES2017",
    "jsx": "react",
    "module": "ESNext",
    "moduleResolution": "node"
  }
}

Playground Link: Provided

asturur · 2023-01-08T08:44:09Z

Some premise, adding items to the prototype isn't hacking.

I thought class properties do not belong to the prototype, only to the instance. Methods belong to the prototype.

Sure for pure state properties that sounds a thumb rule to which you can everyone agree with.
That does not work if you don't want to define the same properties along many classes or refactor entirely your code to have it work with this rule.

ShaMan123 · 2023-01-08T08:45:58Z

I need an example.
Static init seems to bridge that gap, does it not?

ShaMan123 · 2023-01-08T08:46:52Z

BTW in the example the static arr property is shared between classes. Pushing 4 to arr in B mutated it for A as well

asturur · 2023-01-08T08:47:25Z

They inerhit just on static things.
You can't put cacheProperties on static in a class, and then grab them in a subclass in a convenient way outside of static.

ShaMan123 · 2023-01-08T14:00:04Z

Thanks for elaborating.
I modified the example you linked. It had a typo, that is why there were js errors.
Regarding 19aa171 I would argue that declare ruined your effort but as you said. Next effort.

class A {
  static method() {
    return 1;
  }
  set(key, value) {
    // TS stop complaining
    if (this.constructor.arr.includes(key)) {
      console.log('ack', key, value);
    }
  }
  static arr = [1,2,3]
  static type = 'A'
}

class B extends A {
  static {
    this.arr.push(4)
    this.type = 'B'
  }
  static arr = [...A.arr, 'key', 'key2'];
  static method() {
    return super.method() + 2;
  }
}

class C extends B {

}

console.log('a', A.method(), A.arr, A.type);
console.log('b', B.method(), B.arr, B.type);
console.log('c', C.method(), C.arr, C.type);

const c = new C();
const b = new B();

C.method = () => {
  console.log('modified static method');
  return 5;
}

c.set('key', 4);
console.log(b.constructor.type)

C.arr = [...C.arr, 'foo'];
c.set('foo', 'bar')
console.log(c.constructor.method())

Output

"use strict";
var _a;
class A {
    static method() {
        return 1;
    }
    set(key, value) {
        // TS stop complaining
        if (this.constructor.arr.includes(key)) {
            console.log('ack', key, value);
        }
    }
}
A.arr = [1, 2, 3];
A.type = 'A';
class B extends A {
    static method() {
        return super.method() + 2;
    }
}
_a = B;
(() => {
    _a.arr.push(4);
    _a.type = 'B';
})();
B.arr = [...A.arr, 'key', 'key2'];
class C extends B {
}
console.log('a', A.method(), A.arr, A.type);
console.log('b', B.method(), B.arr, B.type);
console.log('c', C.method(), C.arr, C.type);
const c = new C();
const b = new B();
C.method = () => {
    console.log('modified static method');
    return 5;
};
c.set('key', 4);
console.log(b.constructor.type);
C.arr = [...C.arr, 'foo'];
c.set('foo', 'bar');
console.log(c.constructor.method());

Compiler Options

{
  "compilerOptions": {
    "strict": true,
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "alwaysStrict": true,
    "esModuleInterop": true,
    "declaration": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "target": "ES2017",
    "jsx": "react",
    "module": "ESNext",
    "moduleResolution": "node"
  }
}

Playground Link: Provided

asturur · 2023-01-08T14:08:32Z

I m not sure declare are doing anything to the code rather than claryfing for tools that those are types and not actual code.
Do you think are doing something else?

ShaMan123 · 2023-01-08T14:27:34Z

I don't know. It is so unclear what happens under the hood. What is taken care by rollup, the plugins and native classes.
Looking at the build file:

ShaMan123

looks fine
a few small comments

ShaMan123 · 2023-01-08T14:04:39Z

src/shapes/Object/InteractiveObject.ts

+      if (
+        this === (target as InteractiveFabricObject) &&
+        action.slice &&
+        action.slice(0, 5) === 'scale'


startsWith?

action.startsWith('scale')

ShaMan123 · 2023-01-08T14:06:21Z

src/shapes/Object/InteractiveObject.ts

+  _updateCacheCanvas() {
+    const targetCanvas = this.canvas;
+    if (this.noScaleCache && targetCanvas && targetCanvas._currentTransform) {
+      const target = targetCanvas._currentTransform.target,
+        action = targetCanvas._currentTransform.action;
+      if (
+        this === (target as InteractiveFabricObject) &&
+        action.slice &&
+        action.slice(0, 5) === 'scale'
+      ) {
+        return false;
+      }
+    }
+    return super._updateCacheCanvas();
+  }


I love this!
It is well scoped.

ShaMan123 · 2023-01-08T14:08:57Z

src/shapes/Object/Object.ts

+  protected setOptions(options: Record<string, any> = {}) {
    this._setOptions(options);
  }


do we really need this alias?
It is called only once in fabric from this contrusctor

and _setOptions is an alias for set so that can go away as well.
too many methods that do the same shit

not this should go because no strange stuf in the constructor. It will go when we move the defaults

ShaMan123 · 2023-01-08T14:11:22Z

src/shapes/Object/Object.ts

+    // static canvas and canvas have both an array of InteractiveObjects
+    // @ts-ignore this needs to be fixed somehow, or ignored globally


I think static canvas should have the non interactive object as a type while canvas should have the interactive object
Wondering how complicated and dumb that might turn out

ShaMan123 · 2023-01-08T14:16:25Z

src/shapes/ellipse.class.ts

-    this.set('rx', (options && options.rx) || 0);
-    this.set('ry', (options && options.ry) || 0);


I am surprised this doesn't break width and height values

because constructor({...}) is doing that up in the chain with setOptions.
The issue could be if you pass with after rx,ry. But those are exactly the kind of things that needs to addressed when we setup the defaults.
I would also not bother, i prefer to say you have to set both of them, and zero is not a good value if not for both. I wouldn't try autofixing for the developer that have to understand this once.

Same issue with polygon bounding box, line points, radius and witdh/height. there are so many things we try to take care of we probably shouldn't.

If it wasn't late now i woudl say that width and height should be internal prop for geometry and the one we set for rect and text should be some sort of public width/height that nothing have to do with the geometry calculcations.

If it wasn't late now i woudl say that width and height should be internal prop for geometry and the one we set for rect and text should be some sort of public width/height that nothing have to do with the geometry calculcations.

Sounds good

Same issue with polygon bounding box, line points, radius and witdh/height. there are so many things we try to take care of we probably shouldn't.

we should draft topics for a team meeting

yes this whole how to use set/ set and _set, side effects, will be a recurring theme

ShaMan123 · 2023-01-08T14:19:55Z

src/shapes/line.class.ts

+    if (isFiller(this.stroke)) {
+      ctx.strokeStyle = this.stroke.toLive(ctx);
+    } else {
+      ctx.strokeStyle = this.stroke ?? ctx.fillStyle;
+    }


this code looks like a util
I think it is duplicated a few times

ShaMan123 · 2023-01-08T14:20:55Z

src/shapes/polygon.class.ts

@@ -1,3 +1,4 @@
+import { IPoint } from '../point.class';


probably a leftover of a previous thing.

Oh right, when it comes to assigning defaults this will have a constructor too. So it will need the IType definititon

ShaMan123 · 2023-01-08T14:22:49Z

src/shapes/textbox.class.ts

+  // with the new controls this noScaleCache here shouldn't be needed anymore
  lockScalingFlip: true,
+  // with the new controls this noScaleCache here shouldn't be needed anymore


care to elaborate?

I think i was thinking at the width behaviour and that scaling does not affect textbox because we usually change the width. but i think i forgot of both axis scaling

ShaMan123 · 2023-01-08T14:23:28Z

src/util/transform_matrix_removal.ts

+export const _assignTransformMatrixProps = (
+  object: FabricObjectWithTransformMatrix
+) => {
+  if (object.transformMatrix) {
+    const { scaleX, scaleY, angle, skewX } = qrDecompose(
+      object.transformMatrix
+    );
+    object.flipX = false;
+    object.flipY = false;
+    object.set('scaleX', scaleX);
+    object.set('scaleY', scaleY);
+    object.angle = angle;
+    object.skewX = skewX;
+    object.skewY = 0;
+  }
+};
+
+/**
+ * This function is an helper for svg import. it removes the transform matrix
+ * and set to object properties that fabricjs can handle
+ * @private
+ * @param {Object} preserveAspectRatioOptions
+ */
+export const _removeTransformMatrix = (


don't know pick one. I think exportin only one is good enough

src/util/types.ts

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

… into default-values-object

ShaMan123 · 2023-01-08T14:52:47Z

@asturur don't have an option in VSCODE that it formats on save running prettier?

ShaMan123 · 2023-01-08T14:55:53Z

look https://github.com/fabricjs/fabric.js/compare/vscode-config-patch

asturur · 2023-01-08T14:55:58Z

@asturur don't have an option in VSCODE that it formats on save running prettier?

Works sporadically for me, both here and at work prettier does not kick in for me.

asturur · 2023-01-08T15:51:34Z

I have mixed the code with the dependency update branch with export and type declaration and i have a local branch with only 101 failing test from 174 i started after merging.
That means the declare are avoiding side effects we didn't plan for.
As soon as i m down to 0 fails it means that this work, i ll put all the declare here and then we can merge it

asturur · 2023-01-08T16:13:33Z

ok down to 9 failures but i have to stop now

asturur · 2023-01-08T17:29:28Z

ok so those additional declare changes ( no test changes, no code changed ) should give us support for export and types emitting, using babel OR the other rollup plugin ( That does not support browserlist )

asturur · 2023-01-09T11:52:47Z

let's give priority to this today if possible, i want to move asap to discussing browerlist file and what we support and transpile on which tool.
Practicing with the new import strategy and see if tree shaking is possible is another big chunk of work that i would like to be available for whoever wants to try it

ShaMan123 · 2023-01-09T12:53:58Z

how can i help?

ShaMan123

I would expect TS to expose a flag in the config controlling this instead of this shitty declare business

ShaMan123 · 2023-01-09T12:59:25Z

src/shapes/textbox.class.ts

@@ -31,7 +31,7 @@ export class Textbox extends IText {
   * Cached array of text wrapping.
   * @type Array
   */
-  declare __cachedLines: Array<any> | null = null;
+  declare __cachedLines: Array<any> | null;


this is a dead prop

asturur · 2023-01-09T19:33:49Z

how can i help?

Usually just approve if the PR is ok, or block if there is something you really don't like

ShaMan123 · 2023-01-10T01:24:06Z

Just saw this shit
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier
https://www.typescriptlang.org/tsconfig#useDefineForClassFields

Is this what was creating this shit storm??

ShaMan123 · 2023-01-10T01:26:57Z

and microsoft/TypeScript#33509

asturur · 2023-01-10T07:12:52Z

Yes i didn't read all the links deeply, but exactly the part of initializing as undefined.
The older ts-plugin was shielding us from that.

We will have to assign our own values with define properties too since we are doing it manually if we want to do that on spec.
Babel does that, and we can take their small initialization function.

ShaMan123 · 2023-01-10T07:19:46Z

So a babel plugin will save us from declare?

asturur added 2 commits January 7, 2023 10:44

remove tscheck from object

d65c829

ok

5e85bf9

save work

82bcebc

should build at least

19aa171

asturur added 10 commits January 7, 2023 20:00

rewind some changes

562ecda

more rewind

67528c0

more rewind

7be45c4

more rewind

2337dbf

more rewind

3891f4c

more rewind

f18b566

missing file

f6a2ff2

wrong syntax

034de7d

more rewind

53f51e6

fixes

f17dd4d

more declare

edb6217

ShaMan123 reviewed Jan 8, 2023

View reviewed changes

asturur and others added 3 commits January 8, 2023 15:41

Update src/util/types.ts

448e6bf

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

change code comments

d47d2f4

Merge branch 'default-values-object' of github.com:fabricjs/fabric.js…

827a278

… into default-values-object

declare on canvas too

e7c3f8e

declare and protected have an order

25e8177

all the declare needed

a160f07

asturur added 2 commits January 8, 2023 23:59

renamed badly named function

1fba395

Merge branch 'master' into default-values-object

b995f09

ShaMan123 approved these changes Jan 9, 2023

View reviewed changes

asturur merged commit fa697ac into master Jan 10, 2023

asturur deleted the default-values-object branch January 10, 2023 01:18

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

chore(TS): Add declare to every non initialized property. (#8574)

77768d1

		// static canvas and canvas have both an array of InteractiveObjects
		// @ts-ignore this needs to be fixed somehow, or ignored globally

		this.set('rx', (options && options.rx) \|\| 0);
		this.set('ry', (options && options.ry) \|\| 0);

chore(TS): Add declare and remove useless methods from Object. #8574

chore(TS): Add declare and remove useless methods from Object. #8574

Conversation

asturur commented Jan 7, 2023 • edited

Motivation

Description

Changes

Gist

In Action

asturur commented Jan 7, 2023

asturur commented Jan 7, 2023 • edited

github-actions bot commented Jan 7, 2023 • edited

asturur commented Jan 7, 2023

asturur commented Jan 7, 2023

ShaMan123 commented Jan 8, 2023

ShaMan123 commented Jan 8, 2023 • edited

ShaMan123 commented Jan 8, 2023 • edited

asturur commented Jan 8, 2023 • edited

ShaMan123 commented Jan 8, 2023

asturur commented Jan 8, 2023

ShaMan123 commented Jan 8, 2023

asturur commented Jan 8, 2023

ShaMan123 commented Jan 8, 2023

ShaMan123 commented Jan 8, 2023

asturur commented Jan 8, 2023

ShaMan123 commented Jan 8, 2023

asturur commented Jan 8, 2023

ShaMan123 commented Jan 8, 2023 • edited

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Jan 8, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur Jan 8, 2023 • edited

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 commented Jan 8, 2023

ShaMan123 commented Jan 8, 2023

asturur commented Jan 8, 2023

asturur commented Jan 8, 2023

asturur commented Jan 8, 2023

asturur commented Jan 8, 2023

asturur commented Jan 9, 2023

ShaMan123 commented Jan 9, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jan 9, 2023

ShaMan123 commented Jan 10, 2023 • edited

ShaMan123 commented Jan 10, 2023

asturur commented Jan 10, 2023

ShaMan123 commented Jan 10, 2023 • edited

asturur commented Jan 7, 2023 •

edited

asturur commented Jan 7, 2023 •

edited

github-actions bot commented Jan 7, 2023 •

edited

ShaMan123 commented Jan 8, 2023 •

edited

ShaMan123 commented Jan 8, 2023 •

edited

asturur commented Jan 8, 2023 •

edited

ShaMan123 commented Jan 8, 2023 •

edited

ShaMan123 Jan 8, 2023 •

edited

asturur Jan 8, 2023 •

edited

ShaMan123 commented Jan 10, 2023 •

edited

ShaMan123 commented Jan 10, 2023 •

edited