Chore(TS) Object interactivity #8401
Conversation
| export function applyMixins(derivedCtor: any, constructors: any[]) { | ||
| constructors.forEach((baseCtor) => { | ||
| Object.getOwnPropertyNames(baseCtor.prototype).forEach((name) => { | ||
| Object.defineProperty( | ||
| derivedCtor.prototype, | ||
| name, | ||
| Object.getOwnPropertyDescriptor(baseCtor.prototype, name) || | ||
| Object.create(null) | ||
| ); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
unused. i commited because i wanted to use it, we can keep it and then trash it if is not useful
| (function (global) { | ||
| const fabric = global.fabric; | ||
| fabric.Object = FabricObject; | ||
| })(typeof exports !== 'undefined' ? exports : window); |
There was a problem hiding this comment.
moved this to InteractiveFabricObject
|
Build Stats
*inaccurate, see link |
| callSuper?: TCallSuper; | ||
|
|
There was a problem hiding this comment.
this is a compatibility layer for classes that now import extended classes that are not created with fabric.createClass
|
|
||
| /** | ||
| * Color of controlling corners of an object (when it's active and transparentCorners false) | ||
| * @since 1.6.2 | ||
| * @type String | ||
| * @default null | ||
| */ | ||
| cornerStrokeColor: string | null; | ||
| cornerStrokeColor: string; |
There was a problem hiding this comment.
empty string has the same effect of null, so we can reduce to string and keep it compatible with the embedded types of ctx.fillStyle and ctx.strokeStyle
There was a problem hiding this comment.
good
I came across that in #8400 as well
src/shapes/fabricObject.class.js
Outdated
| import { | ||
| InteractiveFabricObject | ||
| } from "../mixins/object_interactivity.mixin"; | ||
|
|
||
| // TODO somehow we have to make a tree-shakeable import | ||
|
|
||
| export { InteractiveFabricObject as FabricObject } | ||
|
|
||
| (function (global) { | ||
| const fabric = global.fabric; | ||
| fabric.Object = InteractiveFabricObject; | ||
| })(typeof exports !== 'undefined' ? exports : window); |
There was a problem hiding this comment.
if we don't find a better way, this together with the dead code remover plugin for rollup can do the optional imports.
There was a problem hiding this comment.
I am currently for the several entry points approach
import { Canvas, Text } from 'fabric/node';
import { Canvas, Text } from 'fabric/static'; // or other name
import { Canvas, Text } from 'fabric'; // interactive by defaultThere was a problem hiding this comment.
well the point is that Rect and Circle have to extend either from Object or Interactive Object, so that is why i get confused on how i change that throug imports.
Yes we can have different entry points that create 3 bundles and people that use the bundle entirely can do that, or they can just import the single TS files as they see fit. But at that point, you have to provide 2 version of Rect/Circle/Text...?
Till i m not done converting i ll ignore it
There was a problem hiding this comment.
A headache indeed
So turn into Mixins, all of them
There was a problem hiding this comment.
Yes but it stays complicated mixin or class is still a bit complicated.
When we have a full ts folder we can worry about thay
|
@ShaMan123 @melchiar this is ready to go |
| * @param {Point} size | ||
| * @param {Object} styleOverride object to override the object style | ||
| */ | ||
| _drawBorders(ctx: CanvasRenderingContext2D, size: Point, styleOverride: Record<string, any> = {}): void { |
There was a problem hiding this comment.
maybe? not sure, at some point we have to decide what becomes private for the sake of reducing exposure Not sure if someone would want to call _drawBorders directly
src/shapes/fabricObject.class.js
Outdated
| import { | ||
| InteractiveFabricObject | ||
| } from "../mixins/object_interactivity.mixin"; | ||
|
|
||
| // TODO somehow we have to make a tree-shakeable import | ||
|
|
||
| export { InteractiveFabricObject as FabricObject } | ||
|
|
||
| (function (global) { | ||
| const fabric = global.fabric; | ||
| fabric.Object = InteractiveFabricObject; | ||
| })(typeof exports !== 'undefined' ? exports : window); |
There was a problem hiding this comment.
I am currently for the several entry points approach
import { Canvas, Text } from 'fabric/node';
import { Canvas, Text } from 'fabric/static'; // or other name
import { Canvas, Text } from 'fabric'; // interactive by default|
|
||
| /** | ||
| * Color of controlling corners of an object (when it's active and transparentCorners false) | ||
| * @since 1.6.2 | ||
| * @type String | ||
| * @default null | ||
| */ | ||
| cornerStrokeColor: string | null; | ||
| cornerStrokeColor: string; |
There was a problem hiding this comment.
good
I came across that in #8400 as well
| * @type number|string|any | ||
| * @default 0 | ||
| */ | ||
| __corner: number | string; |
There was a problem hiding this comment.
why not make all 0 assignments null or undefined?
There was a problem hiding this comment.
yes i thought the same, we could move it to 0 as default rather than null. I don't think anyone is gonna cry for that.
There was a problem hiding this comment.
I am confused
I thought removing 0 in favor of null/undefined
There was a problem hiding this comment.
sorry nor zero nor null. just empty string or optional with undefined.
i din't like null it often makes no sense to me.
There was a problem hiding this comment.
I am asking since I saw the code still has __corner = 0
There was a problem hiding this comment.
either empty string or undefined i suppose. With optional string ( and so undefined ) looking more clean
|
I see you missed prettier and changelog, they are failing |
@asturur I have no idea what to do with the public fields
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Motivation
Continuing conversiont to TS.
Description
Continue the TS migration, change to imports.
For now the mixin is not optional.
Ideally we can split in this way:
importing static canvas adds the stacking mixin
importing canvas adds staticCanvas and the interactivity mixin + controls.
I m not yet sure how the optional import is supposed to work
Changes
BREAKING:
Gist
In Action