-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(TS) Converting Object to a class #8322
Conversation
src/mixins/observable.mixin.ts
Outdated
fabricHeader.Observable = { | ||
fire: fire, | ||
on: on, | ||
once: once, | ||
off: off, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is some bullshit i did when i was half asleep.
I didn't even remember doing it.
Going to delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that in #8103 and removed that there as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like i added it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff is too harsh.
@@ -93,7 +90,7 @@ export class Gradient< | |||
gradientTransform, | |||
id, | |||
}: GradientOptions<T>) { | |||
const uid = fabric.Object.__uid++; | |||
const uid = FabricObject.__uid++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not the place for the __uid. i remember @ShaMan123 already worked to put it somewhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
continue; | ||
} | ||
attributes[attr] = fabric.Object.prototype[attr]; | ||
attributes[attr] = FabricObject.prototype[attr]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it should be exposed as a static method on FabricObject...?
Doesn't sound it will fix anything.
This is a design thing...
|
||
if (parent) { | ||
Subclass.prototype = parent.prototype; | ||
klass.prototype = new Subclass(); | ||
parent.subclasses.push(klass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this makes a lot of tests fail
Textbox etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (not sure) that callSuper uses something of that logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShaMan123 callSuper uses superclasses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is it for extend?
I can't scan the codebase currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it was unused
Maybe and yes. 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.
|
|
the fact is that the cache properties may be definied per class, but accessed by the base class. |
Co-authored-by: ShaMan123 <[email protected]> Co-authored-by: Shachar <[email protected]>
src/shapes/object.class.ts
Outdated
* @fires drop | ||
*/ | ||
|
||
const fabricObjectDefaultValues = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe give this object a type? like const fabricObjectDefaultValues: FabricObject
or const fabricObjectDefaultValues: Partial<FabricObject>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again i want to be able, as today, to change default values at runtime. |
|
Finally I get it. |
You are talking about reactivity. Like mobx. Fabric should not do that. |
In other words, to make myself clear, a shifting/shared class state is an anti pattern |
Fabric won't use it in the codebase. |
but set is not giving any functionality that can't be obtained with similar code. 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. |
I will reply in our chat! |
3e93f57
to
65e4c33
Compare
UPDATEAfter 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:
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. |
* @default undefined | ||
* @private | ||
*/ | ||
_cacheContext: CanvasRenderingContext2D | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a public field, this thing is either null or not and does not need to be configured ever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is public for what reason?
Context is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean is not public, it can be private. But it does need to be in the prototype ever
* @default undefined | ||
* @private | ||
*/ | ||
canvas?: StaticCanvas | Canvas; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the types below this one are new and added reading the code of object.class
…cjs/fabric.js into converting-object-to-class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot these
import { createCanvasElement } from '../util/misc/dom'; | ||
|
||
type StaticCanvas = any; | ||
type Canvas = any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the Canvas type from __types__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as soon as Canvas is converted, we can trash those all away
type StaticCanvas = any; | ||
type Canvas = any; | ||
|
||
const ALIASING_LIMIT = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract to config? or constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
* Observable changes to TS class from @ShaMan123 (fabricjs#8330) Co-authored-by: ShaMan123 <[email protected]> Co-authored-by: Shachar <[email protected]>
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:
The methods changed from:
to
Nothing else has been touched
All the properties are at the end of the file and are assigned like this:
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
Todos