chore(TS) Add more types to Text, BREAKING: change return type of _getStyleDeclaration #9018

asturur · 2023-06-14T04:34:08Z

Motivation

I start to not feel confindent with large classes having ts-nocheck still in place.
Especially text.

Those types i did are not perfect, but better than nothing.

Before merging, discussing if we should get rid of stroke and fill being null since it doesn't makes much sense.
We should understand what null does today ( skill fill? ) and use maybe transparent in place of it and skip fill when transparent is used.
It simplify types a lot and makes every fill not skipped safe anyway.

github-actions · 2023-06-14T04:36:17Z

Build Stats

file / KB (diff)	bundled	minified
fabric	922.881 (+0.288)	303.173 (-0.093)

asturur · 2023-06-15T00:55:58Z

still some work to do here, not ready for review

src/shapes/IText/IText.ts

asturur · 2023-06-15T00:58:06Z

src/shapes/IText/IText.ts

@@ -668,9 +676,9 @@ export class IText<
   * Unused by the library, is for the end user
   * @return {String | TFiller} Character color (fill)
   */
-  getCurrentCharColor(): string | TFiller {
+  getCurrentCharColor(): string | TFiller | null {


fill and stroke being null is the cause of many issue with types and a lot of extra null checks.

if we removing null why this this needed?

because i m not removing null in this PR.
But this PR brought up the issue again

asturur · 2023-06-15T00:59:46Z

src/shapes/Text/StyledText.ts

    const { lineIndex, charIndex } = this.get2DCursorLocation(index);

    if (!this._getLineStyle(lineIndex)) {
      this._setLineStyle(lineIndex);
    }

-    if (!this._getStyleDeclaration(lineIndex, charIndex)) {
+    if (!Object.keys(this._getStyleDeclaration(lineIndex, charIndex)).length) {


this is the only code change i made.
there were 5 occurrencies in which returning undefined from _getStyleDeclaration needed to be fixed with an || {} while just one where returning undefined ( or null ) was useful, to know if there was no style.

I guess this is fine but BREAKING if anyone used it before

i would make this breaking, and i someone asks, i would add the hasStyleDeclaration method that return true/false, or maybe i can add it right away

I think you should
Then there is a clear migration path (Object.keys is pretty clear also but...)

asturur · 2023-06-15T01:00:42Z

src/shapes/Text/StyledText.ts

    const lineStyle = this.styles && this.styles[lineIndex];
-    return lineStyle ? lineStyle[charIndex] : null;
+    return lineStyle ? lineStyle[charIndex] : {};


this is the code change that caused the object.keys above.

asturur · 2023-06-15T01:03:07Z

src/shapes/Text/Text.ts

@@ -63,20 +69,16 @@ type TPathAlign = 'baseline' | 'center' | 'ascender' | 'descender';
 * needs the the info of previous graphemes already filled
 * Override to customize measuring
 */
-export type GraphemeBBox<onPath = false> = {
+export type GraphemeBBox = {


this was too hard to be flexible ( or simply i can't use it )
I m thinking that maybe we just keep al the properties required and we just add the extra 3 everywhere.

asturur · 2023-06-15T01:06:51Z

src/shapes/Text/Text.ts

@@ -1528,22 +1566,23 @@ export class Text<
      const lineLeftOffset = this._getLineLeftOffset(i);
      let boxStart = 0;
      let boxWidth = 0;
-      let lastDecoration = this.getValueOfPropertyAt(i, 0, type);
-      let lastFill = this.getValueOfPropertyAt(i, 0, 'fill');
+      let lastDecoration = this.getValueOfPropertyAt<typeof type>(i, 0, type);


typeof type is fantastic

asturur · 2023-06-15T01:07:12Z

src/shapes/Text/Text.ts

-      textContent = element.textContent;
-    }
-
-    textContent = textContent


ie9 support is gone since long.

asturur · 2023-06-15T01:07:57Z

src/shapes/Text/constants.ts

@@ -87,7 +87,7 @@ export const textDefaultValues: Partial<TClassProperties<Text>> = {
  textBackgroundColor: '',
  stroke: null,
  shadow: null,
-  path: null,
+  path: undefined,


not sure why null was chose in first place.

asturur · 2023-06-15T01:08:17Z

src/shapes/Textbox.ts

@@ -244,7 +252,7 @@ export class Textbox extends IText {
   * @param {Object} style
   * @private
   */
-  _setLineStyle(lineIndex: number) {
+  protected _setLineStyle(lineIndex: number) {


those are protected in the other classes

github-actions · 2023-06-15T01:26:58Z

Coverage after merging text-type-more into master will be

83.75%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	118, 129, 138, 31, 94
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 37, 44, 51, 58, 65, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79.05%	77.54%	83.05%	79.57%	1000–1001, 1001, 1001–1003, 1005–1006, 1006, 1006, 1008, 1016, 1016, 1016–1018, 1018, 1018, 1024–1025, 1033–1034, 1034, 1034–1035, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1515, 161, 186, 296–297, 300–304, 309, 332–333, 338–343, 36, 363, 363, 363–364, 364, 364–365, 373, 378–379, 379, 379–380, 382, 391, 397–398, 398, 398, 40, 441, 449, 453, 453, 453–454, 456, 538–539, 539, 539–540, 546, 546, 546–548, 568, 570, 570, 570–571, 571, 571, 574, 574, 574–575, 578, 587–588, 590–591, 593, 593–594, 596–597, 609–610, 610, 610–611, 613–618, 624, 631, 668, 668, 668, 670, 672–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 741, 741, 799–800, 800, 800–801, 803, 806–807, 807, 807–808, 810–811, 814, 814–816, 819–820, 890, 902, 909, 930, 962, 983–984
SelectableCanvas.ts	94.42%	92.24%	94.23%	96.15%	1115, 1115–1116, 1119, 1139, 1139, 1188, 1196, 1315, 1317, 1319–1320, 519, 694–695, 697–698, 698, 698, 747–748, 809–810, 863–865, 897, 902–903, 930–931
StaticCanvas.ts	96.73%	92.88%	100%	98.52%	1037–1038, 1038, 1038–1039, 1159, 1169, 1223–1224, 1227, 1262–1263, 1339, 1348, 1348, 1352, 1352, 1399–1400, 309–310, 326, 694, 706–707
TextEditingManager.ts	84.31%	71.43%	91.67%	88%	17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	100%	100%	100%	100%
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	93.33%	87.88%	91.67%	97.78%	175, 240, 327, 327, 362
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	5.97%	0%	0%	11.11%	100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.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.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	74.07%	33.33%	66.67%	88.89%

asturur · 2023-06-15T01:46:51Z

This is ready to be discussed @ShaMan123 @melchiar also @jiayihu since you are around TS.
This is not meant to be perfect, the goal is to remove ts-nocheck from as much files as possible

asturur · 2023-06-15T02:04:05Z

test/unit/text.js

@@ -59,7 +59,7 @@
    skewY:                      0,
    charSpacing:                0,
    styles:                     [],
-    path:                       null,
+    path:                       undefined,


i wonder if i need to make a backward compatibilitu statement or change somewhere.
The code never check if path is undefined, all it does is checking if path is truthy or not.
So null or undeined is the same also when reloading old data

ShaMan123

I want to understand the problem with GraphemeBBox<true | false>
generic getValueOfPropertyAt

Apart from that looks good

ShaMan123 · 2023-06-15T03:14:25Z

src/shapes/IText/IText.ts

@@ -668,9 +676,9 @@ export class IText<
   * Unused by the library, is for the end user
   * @return {String | TFiller} Character color (fill)
   */
-  getCurrentCharColor(): string | TFiller {
+  getCurrentCharColor(): string | TFiller | null {


if we removing null why this this needed?

ShaMan123 · 2023-06-15T03:15:05Z

src/shapes/Text/StyledText.ts

    const { lineIndex, charIndex } = this.get2DCursorLocation(index);

    if (!this._getLineStyle(lineIndex)) {
      this._setLineStyle(lineIndex);
    }

-    if (!this._getStyleDeclaration(lineIndex, charIndex)) {
+    if (!Object.keys(this._getStyleDeclaration(lineIndex, charIndex)).length) {


I guess this is fine but BREAKING if anyone used it before

ShaMan123 · 2023-06-15T03:17:20Z

src/shapes/Text/StyledText.ts

    const lineStyle = this.styles && this.styles[lineIndex];
-    return lineStyle ? lineStyle[charIndex] : null;
+    return lineStyle ? lineStyle[charIndex] ?? {} : {};


Are we ok with abbreviations now or not yet?

Suggested change

return lineStyle ? lineStyle[charIndex] ?? {} : {};

return lineStyle?.[charIndex] ?? {};

is so much longer the abbreviation because safari.

i m also not sure about performance in general of that abbrevieation

src/shapes/Text/Text.ts

src/shapes/Textbox.ts

ShaMan123 · 2023-06-15T03:38:04Z

src/shapes/Textbox.ts

    if (this._styleMap && !this.isWrapping) {
      const map = this._styleMap[lineIndex];
      if (!map) {
-        return null;
+        return {};
      }
      lineIndex = map.line;
      charIndex = map.offset + charIndex;


though this is not related I would like to say that this piece of logic has a lot more usages than just in style.
It maps between a wrapped position to a non wrapped position.
It is confusing when to use the wrapped/non wrapped position when working on textbox, error prone and not friendly.
I would have a method that returns the offset give a line, char, isWrapped since it is not changed by wrapping (I hope there is no edge case for that) and use that across all methods as a standard.
This will make working on Text and Textbox easier or else we must keep in mind this constant difference that is applied at a certain point in the lifecycle. e.g. measuring uses to non wrapped, rendering uses the wrapped.
All should us the offset.

as i said there are so many things that have been bolted on top of normal text, and this stylemap was one.
This works, is confusing, but it works,

src/parser/applyViewboxTransform.ts

src/parser/doesSomeParentMatch.ts

asturur · 2023-06-15T07:00:11Z

Removed some extra low hanging fruits and broke test.

asturur · 2023-06-15T07:58:12Z

I want to understand the problem with GraphemeBBox<true | false>

GraphemeBBox starts as but then later is true, and there is no easy way to inform the code that it has been updated because is done with mutation through different methods

asturur · 2023-06-15T07:59:03Z

Needs to decide what to do with the breaking change.

ShaMan123 · 2023-06-15T09:54:08Z

I want to understand the problem with GraphemeBBox<true | false>

GraphemeBBox starts as but then later is true, and there is no easy way to inform the code that it has been updated because is done with mutation through different methods

I would make that not mutate but return a new object regardless of types
But that is for another cleanup

asturur · 2023-06-15T12:33:11Z

I want to understand the problem with GraphemeBBox<true | false>

GraphemeBBox starts as but then later is true, and there is no easy way to inform the code that it has been updated because is done with mutation through different methods

I would make that not mutate but return a new object regardless of types But that is for another cleanup

i don't want to accomodate the logic to types, and we still don't have a benchmark strategy either.
Imagine what surprise if everything we did is 25% slower for a reason or another

ShaMan123 · 2023-06-15T13:02:01Z

Good point

ShaMan123

left overs

ShaMan123 · 2023-06-15T09:56:13Z

src/canvas/DOMManagers/CanvasDOMManager.ts

+      left: '0',
+      top: '0',
    });


I really do not like this
You should use CSS type and this will be resolved

hmm I see that in CCSStyleDeclration that is string anyways.

setAttribute wants strings.
is call setStyles but we are not setting the CSS we are setting the attributes

src/util/dom_style.ts

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

asturur · 2023-06-17T13:35:01Z

i want to merge this and move forward with more types.

ShaMan123 · 2023-06-17T15:40:52Z

Go ahead!

i did this

8456471

more changes

fd44b22

asturur force-pushed the text-type-more branch from 6a4527b to fd44b22 Compare June 14, 2023 04:59

asturur commented Jun 15, 2023

View reviewed changes

src/shapes/IText/IText.ts Outdated Show resolved Hide resolved

asturur commented Jun 15, 2023

View reviewed changes

src/shapes/Text/Text.ts

textContent = element.textContent;

}

textContent = textContent

Copy link

Member Author

asturur Jun 15, 2023

Choose a reason for hiding this comment

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

ie9 support is gone since long.

asturur commented Jun 15, 2023

View reviewed changes

fixed tests

4c59ab8

asturur added 2 commits June 15, 2023 03:48

Merge branch 'master' into text-type-more

3873c95

updated master

d34b151

asturur commented Jun 15, 2023

View reviewed changes

import type

b6de41b

ShaMan123 reviewed Jun 15, 2023

View reviewed changes

asturur added 3 commits June 15, 2023 08:23

fabric imports

7e209cd

test this

8b1b41e

removed a bunch of small ts-nocheck

ee32043

asturur commented Jun 15, 2023

View reviewed changes

src/parser/applyViewboxTransform.ts Show resolved Hide resolved

asturur commented Jun 15, 2023

View reviewed changes

src/parser/doesSomeParentMatch.ts Show resolved Hide resolved

seems ok

c3a8869

ShaMan123 reviewed Jun 16, 2023

View reviewed changes

asturur and others added 5 commits June 16, 2023 13:30

Update src/util/dom_style.ts

316497c

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Merge branch 'master' into text-type-more

33a5eeb

Merge branch 'master' into text-type-more

a3f55fa

changes

c7fffcf

no test changes

bc573e8

asturur changed the title ~~chore(TS) remove ts-nocheck from Text~~ chore(TS) Add more types to Text, BREAKING: change return type of _getStyleDeclaration Jun 17, 2023

asturur mentioned this pull request Jun 17, 2023

🧐 Breaking changes for fabric 6.0 #8299

Open

Merge branch 'master' into text-type-more

c3e8a0e

asturur merged commit cc7b245 into master Jun 19, 2023

asturur deleted the text-type-more branch June 19, 2023 03:06

	return lineStyle ? lineStyle[charIndex] ?? {} : {};
	return lineStyle?.[charIndex] ?? {};

chore(TS) Add more types to Text, BREAKING: change return type of _getStyleDeclaration #9018

chore(TS) Add more types to Text, BREAKING: change return type of _getStyleDeclaration #9018

Conversation

asturur commented Jun 14, 2023

Motivation

github-actions bot commented Jun 14, 2023 • edited Loading

asturur commented Jun 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 15, 2023 • edited Loading

asturur commented Jun 15, 2023

Choose a reason for hiding this comment

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jun 15, 2023

asturur commented Jun 15, 2023

asturur commented Jun 15, 2023

ShaMan123 commented Jun 15, 2023 • edited Loading

asturur commented Jun 15, 2023

ShaMan123 commented Jun 15, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jun 17, 2023

ShaMan123 commented Jun 17, 2023

github-actions bot commented Jun 14, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 commented Jun 15, 2023 •

edited

Loading