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

chores(TS) Remove more empty declarations #8593

Merged
merged 3 commits into from Jan 12, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented Jan 12, 2023

Motivation

Consistency in property declaration in classes.
Avoid transpilers to pre-define and override definitions with undefined of certain props.

The issue to recap was:

class A {
  constructor(options) {
    ...setOptions
  }
}

class B extends A {
 
  myProperty: boolean;
  constructor(options) {
    super(...optionsB ...optionsA);
    // at this point myProperty has been set by the constructor of A
    // this below line is killing the work of constructor A
    defineProp(myProperty, undefined)
  }
}

This may be because our setOptions or other constructor practices are bad patterns, but we are not drilling into that now, this is a simpe way to be safe.
On top of that babel is doing this transpilation, so actually adding a property without declare or without a default value is a TS error that we are avoiding because we a flag in the compiler turned down.
So maybe turning that flag up is another thing we want to do.

Description

Make all property initialization the same.
If a property needs to be defined in the constructor at runtime, we will need to add declare in front of it, regardless that tests aren't failing.
At least we are consistent and we don't have unexpected behaviours only for some properties.

Changes

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2023

Coverage after merging remove-more-empty-declarations into master will be

83.23%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.ts100%100%100%100%
src
   cache.ts96.97%90%100%100%56
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   env.ts72.73%53.33%100%79.17%22, 22–23, 23, 23, 25, 25, 27, 29, 31–32, 62
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.19%85.71%100%96.30%118, 124, 135, 144, 96
   point.class.ts100%100%100%100%
   shadow.class.ts98.48%96%100%100%156
   typedefs.ts100%100%100%100%
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 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
   pattern_brush.class.ts97.06%87.50%100%100%21
   pencil_brush.class.ts91.81%85.42%100%93.52%122–123, 152, 152–154, 276, 280, 285–286, 68–69, 84–85
   spray_brush.class.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
   TextEditingManager.ts100%100%100%100%
   canvas.class.ts92.63%88.68%94%95.32%1151, 1151–1152, 1155, 1168–1170, 1175, 1175, 1237, 1273–1274, 1293–1294, 1302–1303, 1324, 1332, 1445–1446, 1448–1449, 1469–1470, 507, 574–575, 580, 590, 719–720, 722–723, 723, 723, 769–770, 831–832, 885–887, 917, 922–923, 952–953
   canvas_events.ts78.34%75.69%82.35%79.49%1005–1006, 1006, 1006–1008, 1010–1011, 1011, 1011, 1013, 1021, 1021, 1021–1023, 1023, 1023, 1029–1030, 1038–1039, 1039, 1039–1040, 1045, 1047, 1077–1079, 1082–1083, 1087–1088, 1204–1206, 1209–1210, 1283, 1403, 147, 1497–1498, 1504, 1508–1509, 1525, 1547, 1594, 1599, 1638, 1647, 172, 320–321, 321, 321–322, 322, 325–329, 334, 336, 349–352, 355, 374, 374, 374–375, 375, 375–376, 384, 389–390, 390, 390–391, 393, 402, 408–409, 409, 409, 445, 449, 449, 449, 449, 449–450, 452, 527, 529, 529, 529–531, 551, 553, 553, 553–554, 554, 554, 557, 557, 557–558, 561, 570–571, 573–574, 576, 576–577, 579–580, 592–593, 593, 593–594, 596–600, 606, 613, 653, 653, 653, 655, 657–661, 667, 673, 673, 673–674, 676, 679, 684, 697, 724, 780–781, 781, 781–782, 784, 787–788, 788, 788–789, 791–792, 795, 795–797, 800–801, 811, 893, 907, 914, 935, 967, 988–989
   static_canvas.class.ts94.67%89.27%97.92%97.10%1118–1119, 1119, 1119–1120, 1240, 1250, 1304–1305, 1308, 1343–1344, 1422, 1431, 1436, 1485–1486, 1714, 1714–1715, 1764, 1767, 1770, 1770, 1770, 1773, 1776, 1776, 1776, 338, 351, 404–405, 407–408, 417, 421–422, 425–426, 878
src/color
   color.class.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
   changeWidth.ts100%100%100%100%
   control.class.ts93.90%88.89%90.91%97.73%235, 319, 319, 354
   controls.render.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   default_controls.ts86.67%66.67%100%100%122, 129
   drag.ts100%100%100%100%
   polyControl.ts6.35%0%0%11.43%100, 105, 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.ts20%12.50%50%22.22%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.41%92.68%100%93.59%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/filters
   2d_backend.class.ts92%83.33%100%93.75%35–36
   FilterBackend.ts88.89%88.89%100%85.71%15–16
   WebGLProbe.ts37.14%40%60%30%28–30, 30, 30–31,

@github-actions
Copy link
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 940.125 (-1.848) 295.693 (-0.917)

@asturur
Copy link
Member Author

asturur commented Jan 12, 2023

Moving forward with this, there isn't really nothing changed nor is optional right now.
We could probably do a second round for service classes ( stroke projection and others ) to have default values that make sense instead of undefined.
That will make types easier and less declare around.
But those defaults are anyway useless because are ment to be overwritten right away ( like an empty array of points for the base brush ) so i m not sure where is the advantage there.

Maybe there is some extra optimization the browsers guts can do when they know a property will likely exists rather than appear from nothing.

Happy to dig more there but absolutely not now

@asturur asturur merged commit c976302 into master Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant