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): class interface for options/brevity #8674

Merged
merged 13 commits into from
Feb 25, 2023
Merged

chore(TS): class interface for options/brevity #8674

merged 13 commits into from
Feb 25, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Feb 8, 2023

Motivation

Using an interface with class is powerful and reduces the file size because docs can be moved to the interface and leave logic not polluted
The other motivation is that it provides a type for options. One would argue that this is one of the most important things for a TS consumer.

Description

Extracted props to ObjectProps interface and removed comments from logic since VSCODE traces them correctly
image

Changes

Gist

In Action

Circle.ts.-.fabric.js.-.Visual.Studio.Code.2023-02-08.03-04-33_Trim.mp4

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Build Stats

file / KB (diff) bundled minified
fabric 889.156 (-10.477) 304.906 (+0.023)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Coverage after merging ts-interface into master will be

83.16%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%18, 21, 24, 30, 33, 36
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.48%96%100%100%156
   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.51%91.63%94.34%96.59%1102, 1102–1103, 1106, 1126, 1126, 1185, 1235–1236, 1257, 1265, 1390, 1392, 1394–1395, 681–682, 684–685, 685, 685, 734–735, 796–797, 850–852, 884, 889–890, 917–918
   StaticCanvas.ts95.96%91.29%100%97.92%1083–1084, 1084, 1084–1085, 1205, 1215, 1269–1270, 1273, 1308–1309, 1386, 1395, 1395, 1399, 1399, 1446–1447, 1661–1662, 282–283, 300, 380–381, 383–384, 745, 757–758, 843
   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.ts19.77%20%30%17.65%100–101, 101, 101–102, 109–112, 112, 112–113, 119, 119, 119–122, 140, 170–175, 179–180, 180, 180–183, 183,

@ShaMan123 ShaMan123 requested a review from asturur February 8, 2023 01:09
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.

done

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

asturur commented Feb 11, 2023

What happens to transpiled unminified js? it looses all comments?
in that case isn't really good.

Let's check how the JS bundle behaves

@ShaMan123
Copy link
Contributor Author

Comments are removed
image

Why are they needed in the bundle?

@asturur
Copy link
Member

asturur commented Feb 13, 2023

i think my question is, who does import the js bundle and has no access to the packaged, does he find the JSDOCS? i think no because those get stripped away with types at this point.

@ShaMan123
Copy link
Contributor Author

I think yes because they have access to fabric.d.ts that is consumed by the code editor

@ShaMan123
Copy link
Contributor Author

Who uses it like that? Can you point it out? Is it a real case?

@ShaMan123
Copy link
Contributor Author

If you install via package manager you have .d.ts
Any other option is what exactly? Browser cdn? That isn't accessible anyways.

@asturur
Copy link
Member

asturur commented Feb 21, 2023

@ShaMan123 we should be able to add any property to a class that is not already defined with a specific type.
so Rect({ top: 'a' }) should be an error, but Rect({ name: 'hello' }) shouldn't be.
And so fromObject and toObject with propertiesToInclude should behave the same.

Can we just add & Record<string, unknown> to make it happen?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 21, 2023

@ShaMan123
Copy link
Contributor Author

Can we continue there?

@asturur
Copy link
Member

asturur commented Feb 21, 2023

so should i close this and look at that?

@ShaMan123
Copy link
Contributor Author

merge this and move there or close this and we work there
did it for scoping

@asturur
Copy link
Member

asturur commented Feb 24, 2023

ok will get to this today and the next, today

@asturur
Copy link
Member

asturur commented Feb 25, 2023

Moving forward with this, the only thing i m not sold on is removing the jsdocs outside the code. The interface is obviously super useful for types and everything.
This will likely need to be done for rect and other parts i imagine but in that case let's create the interface first and move away the JSDodcs after.
If for some reasons we want jsdoc back in the codebase at least we can just revert the relevant commits

@asturur asturur merged commit ff3edd7 into master Feb 25, 2023
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.

2 participants