Skip to content
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): types for toObject and additionaProperties #8677

Merged
merged 35 commits into from
Mar 4, 2023
Merged

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Feb 8, 2023

Motivation

#8674 opens up to possibility to type toObject

Description

TS limitations of static signatures prevents me from declaring an overload to Circle.fromObject
I decided to override though there is an option to declare the lass in a .d file and change the static signature there I think.
I researched it too long.
Since we are talking about 50 classes or more it sounds better to have a .d file handle it.

Changes

A lot of types
Moved declare fields around

Gist

In Action

Try the following, test autocomplete in new, toObject, fromObject

import { Circle, Object as FabricObject } from "fabric";

class Circle2 extends Circle {
  declare foo: string;
}

const { foo, angle } = new FabricObject({ foo: "bar" }).toObject(["foo"]);
const { foo: bar, fill } = new FabricObject<{ foo: string }>().toObject([
  "foo",
]);

foo;
angle;
bar;
fill;

new Circle({}).fill;
new Circle({}).toObject().fill;

new Circle().toObject(["foo"]).foo;

new Circle({
  radius: 5,
  foo: "bar",
}).toObject(["foo"]).foo;

new Circle2().toObject(["foo"]).foo;

Circle.fromObject({
  a: "b",
  clipPath: {
    absolutePositioned: true,
    inverted: false,
    opacity: 1,
  },
  radius: 5,
});
index.copy.tsx.-.fabTest.-.Visual.Studio.Code.2023-02-08.14-23-05_Trim.mp4

@ShaMan123 ShaMan123 changed the title Types to object fix(): toObject types Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Build Stats

file / KB (diff) bundled minified
fabric 891.435 (-0.067) 304.512 (-0.016)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Coverage after merging types-to-object into master will be

83.19%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 29, 32, 35
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts96.28%96.43%90%98.04%206–207, 232–233
   CommonMethods.ts100%100%100%100%
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   Pattern.ts95.77%94.29%100%96.43%124, 135, 144
   Point.ts100%100%100%100%
   Shadow.ts98.51%96%100%100%168
   cache.ts97.06%90%100%100%56
   config.ts75%64.29%66.67%82.76%138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
   typedefs.ts100%100%100%100%
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.01%82.35%100%93.75%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
   Canvas.ts78.93%77.41%81.67%79.60%1000, 1000, 1000–1002, 1004–1005, 1005, 1005, 1007, 1015, 1015, 1015–1017, 1017, 1017, 1023–1024, 1032–1033, 1033, 1033–1034, 1039, 1041, 1072–1074, 1077–1078, 1082–1083, 1197–1199, 1202–1203, 1276, 1396, 1519, 1589, 161, 186, 295–296, 299–303, 308, 331–332, 337–342, 36, 362, 362, 362–363, 363, 363–364, 372, 377–378, 378, 378–379, 381, 390, 396–397, 397, 397, 40, 440, 448, 452, 452, 452–453, 455, 537–538, 538, 538–539, 545, 545, 545–547, 567, 569, 569, 569–570, 570, 570, 573, 573, 573–574, 577, 586–587, 589–590, 592, 592–593, 595–596, 608–609, 609, 609–610, 612–616, 622, 629, 669, 669, 669, 671, 673–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 740, 740, 798–799, 799, 799–800, 802, 805–806, 806, 806–807, 809–810, 813, 813–815, 818–819, 889, 901, 908, 929, 961, 982–983, 999
   SelectableCanvas.ts94.54%91.63%94.44%96.61%1138, 1138–1139, 1142, 1162, 1162, 1221, 1271–1272, 1293, 1301, 1426, 1428, 1430–1431, 717–718, 720–721, 721, 721, 770–771, 832–833, 886–888, 920, 925–926, 953–954
   StaticCanvas.ts95.98%91.29%100%97.93%1112–1113, 1113, 1113–1114, 1234, 1244, 1298–1299, 1302, 1337–1338, 1415, 1424, 1424, 1428, 1428, 1475–1476, 1690–1691, 311–312, 329, 409–410, 412–413, 774, 786–787, 872
   TextEditingManager.ts100%100%100%100%
src/color
   Color.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   Control.ts93.90%88.89%90.91%97.73%235, 319, 319, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts87.50%100%66.67%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   defaultControls.ts77.78%66.67%100%100%10, 17
   drag.ts100%100%100%100%
   polyControl.ts6.15%0%0%11.11%100, 105, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–28, 30, 30, 30, 30, 30, 30, 30, 30, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts19.57%12.50%50%21.43%41, 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
   scale.ts93.49%92.77%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%77.78%50%100%14, 17
   index.ts91.67%50%100%100%20
   node.ts76.92%50%66.67%88.24%27, 31–32, 32, 32
src/filters
   BaseFilter.ts21.51%22.81%32%18.27%106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182, 182, 182, 182,

@ShaMan123 ShaMan123 added the types Typescript related label Feb 8, 2023
@asturur
Copy link
Member

asturur commented Feb 11, 2023

As usual the important thing is that we can still export any property that has been added by the developer.
What is the status on this?

@ShaMan123
Copy link
Contributor Author

image

@asturur
Copy link
Member

asturur commented Feb 25, 2023

@ShaMan123 i m not sure i can solve the conflicts correctly, the TS declarations confuses me at this point.
I have a couple of comments reading the code:

  • we must be careful of not assuming that serialized props are the same for constructor. It seems to me that Shadow and Gradient could be points of confusions.
  • if the standard fabric object can have a generic type with extra signautures, Circle<any, any, any>, i wish the extra props for the user would be the first one, so i can easily do Circle and letting the rest as the optionals.

@asturur
Copy link
Member

asturur commented Feb 25, 2023

ok i tried with the conflicts. in case revert my commit


static fromObject(object: SerializedCircleProps): Promise<Circle> {
return super.fromObject(object) as Promise<Circle>;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something i noticed in the previous PR too, i would argue this can be solved with a declaration?
is weird that TS would force me to add code that does nothing to obey types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct
I was lazy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another TS problem. Static class methods can't be generic using straight forward TS

padding: number;
}

export interface ObjectProps extends ObjectBaseProps {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i must have done something wrong because i don't see where this part went

@asturur
Copy link
Member

asturur commented Feb 25, 2023

@ShaMan123 please i want to move on with this, so if you have spare time to work on fabric, instead of working on other prs prioritize this one. let's understand how this should be done and then eventually i can complete it for the other classes if you don't have time.

@asturur
Copy link
Member

asturur commented Feb 25, 2023

for today i m off

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/shapes/Object/ObjectProps.ts turned into src/shapes/Object/types

recommended review order:

Types Classes
src/shapes/Object/types/BaseProps.ts src/shapes/Object/ObjectOrigin.ts
src/shapes/Object/types/SerializedObjectProps.ts, src/shapes/Object/types/ObjectProps.ts src/shapes/Object/Object.ts
src/shapes/Object/types/FabricObjectProps.ts src/shapes/Object/InteractiveObject.ts
src/shapes/Object/types/index.ts src/shapes/Object/FabricObject.ts
src/shapes/Circle.ts

Each type file depends on more types so review them as part of it

Comment on lines +7 to +54
cornerSize: number;

/**
* Size of object's controlling corners when touch interaction is detected
* @type Number
* @default 24
*/
touchCornerSize: number;

/**
* When true, object's controlling corners are rendered as transparent inside (i.e. stroke instead of fill)
* @type Boolean
* @default true
*/
transparentCorners: boolean;

/**
* Color of controlling corners of an object (when it's active)
* @type String
* @default rgb(178,204,255)
*/
cornerColor: string;

/**
* Color of controlling corners of an object (when it's active and transparentCorners false)
* @since 1.6.2
* @type String
* @default null
*/
cornerStrokeColor: string;

/**
* Specify style of control, 'rect' or 'circle'
* This is deprecated. In the future there will be a standard control render
* And you can swap it with one of the alternative proposed with the control api
* @since 1.6.2
* @type 'rect' | 'circle'
* @default rect
* @deprecated
*/
cornerStyle: 'rect' | 'circle';

/**
* Array specifying dash pattern of an object's control (hasBorder must be true)
* @since 1.6.2
* @type Array | null
*/
cornerDashArray: number[] | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must say that corner is bad naming, should be control

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 27, 2023

updated desc, ready
rest of shapes in followup

Comment on lines +475 to +478
toObject<
T extends Omit<Props & TClassProperties<this>, keyof SProps>,
K extends keyof T = never
>(propertiesToInclude?: K[]): { [R in K]: T[K] } & SProps {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing is unreadable and who invented typescript is responsible for my eyes bleeding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not easy to write and definately not to test

@asturur
Copy link
Member

asturur commented Mar 4, 2023

i had the laptop offline for all day because after a forced update it wasn't starting anymore ( i think the battery went off during update ).
The PR seems ok to me i will merge it and try to do another shape ( like RECT? ) to be sure i understand all the changes properly.

@asturur asturur changed the title fix(): toObject types chore(TS): types for toObject and additionaProperties Mar 4, 2023
@asturur asturur merged commit a0f8275 into master Mar 4, 2023
@asturur asturur deleted the types-to-object branch March 4, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types Typescript related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants