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): Remove stateful mixin #8663

Merged
merged 6 commits into from
Feb 5, 2023
Merged

chore(TS): Remove stateful mixin #8663

merged 6 commits into from
Feb 5, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented Feb 5, 2023

Motivation

The Stateful mixin had 2 defects.

  1. it tried to detect change on objects on the fly ( complex and not good for performances )
  2. It had some weird comparing code we don't want to maintain

The direction

Description

  • remove stateful mixin and its tests

Changes

  • renamed _dimensionAffectingProps to textLayoutProperties, static.
  • changes to textLayoutProperties require the use of set, exactly as cache properties

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

Build Stats

file / KB (diff) bundled minified
fabric 893.528 (-6.870) 304.112 (-1.132)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

Coverage after merging remove-stateful-2 into master will be

83.01%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
fabric.ts100%100%100%100%
index.node.ts7.69%100%0%14.29%18, 21, 24, 30, 33, 36
src
   Collection.ts95.65%95%89.66%97.83%205–206, 231–232
   CommonMethods.ts100%100%100%100%
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%138–139, 164–165, 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.91%77.01%83.08%79.65%1009–1010, 1010, 1010–1012, 1014–1015, 1015, 1015, 1017, 1025, 1025, 1025–1027, 1027, 1027, 1033–1034, 1042–1043, 1043, 1043–1044, 1049, 1051, 1082–1084, 1087–1088, 1092–1093, 1210–1212, 1215–1216, 1289, 1409, 1503–1504, 1510, 1514–1515, 1531, 1553, 1600, 1605, 1653, 171, 196, 305–306, 309–313, 318, 341–342, 347–352, 372, 372, 372–373, 373, 373–374, 38, 382, 387–388, 388, 388–389, 391, 400, 406–407, 407, 407, 42, 450, 458, 462, 462, 462–463, 465, 547–548, 548, 548–549, 555, 555, 555–557, 577, 579, 579, 579–580, 580, 580, 583, 583, 583–584, 587, 596–597, 599–600, 602, 602–603, 605–606, 618–619, 619, 619–620, 622–626, 632, 639, 679, 679, 679, 681, 683–687, 693, 699, 699, 699–700, 702, 705, 710, 723, 750, 750, 808–809, 809, 809–810, 812, 815–816, 816, 816–817, 819–820, 823, 823–825, 828–829, 899, 911, 918, 939, 971, 992–993
   SelectableCanvas.ts93.82%90.71%94.12%96.11%1133, 1133–1134, 1137, 1157, 1157, 1216, 1266–1267, 1288, 1296, 1409–1410, 1412–1413, 1433–1434, 552–553, 558, 568, 701–702, 704–705, 705, 705, 751–752, 813–814, 867–869, 899, 904–905, 934–935
   StaticCanvas.ts95.85%91.04%100%97.92%1083–1084, 1084, 1084–1085, 1205, 1215, 1269–1270, 1273, 1308–1309, 1386, 1395, 1395, 1399, 1399, 1446–1447, 1661, 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%
   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.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.ts93.75%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,

Copy link
Contributor

@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.

Looks great
Can stateProps and cacheProps become static as well??

@@ -149,6 +150,8 @@ export class IText<
*/
declare caching: boolean;

static textLayoutProperties = Text.textLayoutProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this
static props are inherited

Copy link
Contributor

Choose a reason for hiding this comment

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

I see #8662

Copy link
Member Author

Choose a reason for hiding this comment

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

are they? my understand is that they are inherited only in static methods.
If is not needed even better.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

class A {
 static m=5
}
class B extends A {}
console.log(B.m)

isAddingPath = isAddingPath || _key === 'path';
}
} else {
needsDims = this._dimensionAffectingProps.indexOf(key) !== -1;
needsDims = this.constructor.textLayoutProperties.includes(key);
Copy link
Member Author

Choose a reason for hiding this comment

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

This thing here remind me of te issue we have with side effects.
sometimes those fit in _set, but if those are slightly expensive we want to run them only once for set.
But this bring us to handle in set both single and multi key use case.
@ShaMan123 should check your side effects pr solves this issue with 10 lines of code or find a valid way to remove those with a little footprint.

Copy link
Contributor

Choose a reason for hiding this comment

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

don't think it is 10 lines but it solves this bad design

@ShaMan123
Copy link
Contributor

ShaMan123 commented Feb 5, 2023

Another thought
We might want to get rid of static fields because they compile to a side effect and a method doesn't, or is it a matter of fine tuning rollup/browserlist?

@asturur
Copy link
Member Author

asturur commented Feb 5, 2023

they compile to a sideEffect you mean they become
Object.prop = value ?
I m going to check what the output is like.

Seems like classes aren't ready if i have to create a getter for a prop because they are handled differently.
It also leaves me with the issue of creating a setter too.

@asturur
Copy link
Member Author

asturur commented Feb 5, 2023

yes so they become side effects.
When we get to code tree shaking we have to decide.
It could also be that we leave the lib unbundled for who wants to optimize the bundle and imports the single files.
The other will use a large premade bundle built with rollup.

I m kind of tired to spinning in circle to js/ts issues
This migration definitely took a toll on me.

@asturur asturur requested a review from ShaMan123 February 5, 2023 14:43
@asturur
Copy link
Member Author

asturur commented Feb 5, 2023

This is ready to go, not tackling side effects now, the scope is removing stateful.

Copy link
Contributor

@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.

Yes
The maintenance is a really unpleasant part.
I looked at the links of tree shaking in rollup
https://rollupjs.org/configuration-options/#treeshake-modulesideeffects

We can expose a rollup config for consumers or whatever
Never mind that lets move fabric forward
It has taken a toll on me too

@asturur asturur merged commit 9258628 into master Feb 5, 2023
ShaMan123 added a commit that referenced this pull request Feb 19, 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.

2 participants