chore(TS): convert StaticCanvas #8485

asturur · 2022-12-03T22:56:55Z

Converting only StaticCanvas for now, Canvas in next PR

added new types for small objects that had recurring usage
had to losen a bit the toObect properties array
gave a stab at reducing and converting the stateful mixin but is not worth, is easier to just delete it
changed some private/protected stuff and add/remove implemementation to make TS happy
BREAKING drawClipPathOnCanvas no takes the clipPath as second argument instead of rechecking its existance inside.
introduced a type for cachedObject to specify which functions actually need an object that has a woring cache ( maybe we will cancel this idea before getting out of beta )

Motivation

Description

Changes

Gist

In Action

github-actions · 2022-12-04T15:57:29Z

Build Stats

file / KB (diff)	bundled	minified
fabric	983.963 (-13.591)	310.080 (-1.294)

github-actions · 2022-12-04T20:30:33Z

Coverage after merging TS-convert-static-canvas into master will be

82.77%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
HEADER.js	54%	48.15%	0%	63.64%	12, 14, 14, 14, 14, 14, 16–17, 21, 21–22, 22, 22, 24, 24, 26, 28, 30–31, 77, 77, 77
src
cache.ts	97.06%	90%	100%	100%	56
canvas.class.ts	93%	89.93%	94.12%	95.06%	1047, 1047–1048, 1051, 1071, 1071, 1106, 1139–1140, 1168–1169, 1202, 1210, 1320–1321, 1323–1324, 1344–1345, 1511, 1516, 1526, 1526, 1526–1527, 1530, 474–475, 480, 489, 638–640, 685–686, 735–736, 739, 741, 784–786, 828, 833–834, 862–863
config.ts	77.27%	66.67%	66.67%	84.62%	130, 138–140, 151–153
constants.ts	100%	100%	100%	100%
intersection.class.ts	100%	100%	100%	100%
pattern.class.ts	92.19%	85.71%	100%	96.30%	118, 124, 135, 144, 96
point.class.ts	100%	100%	100%	100%
shadow.class.ts	98.51%	96%	100%	100%	157
static_canvas.class.ts	92.22%	86.67%	96.88%	94.64%	1186–1187, 1187, 1187–1188, 1308, 1318, 1372–1373, 1376, 1411–1412, 1490, 1499, 1504, 1619, 1621, 1623, 1623, 1623, 1623, 1626, 1626, 1626–1627, 1679–1681, 1683, 1685, 1685, 1685, 1685, 1689, 1689, 1689–1691, 1762, 1762–1763, 1820, 1822–1823, 1823, 1823, 1826–1827, 1827, 1827, 280, 302–306, 322, 392–393, 395–396, 405, 409–410, 412–413, 930
src/brushes
base_brush.class.ts	100%	100%	100%	100%
circle_brush.class.ts	1.52%	0%	0%	2%	101, 103–105, 114, 114, 114, 116, 118, 120–122, 124–127, 135, 142, 144, 24, 29–30, 38–42, 46–50, 57–60, 68–72, 74, 82, 82, 82, 82, 82–83, 85, 85, 85–88, 90, 98–99
pattern_brush.class.ts	97.14%	87.50%	100%	100%	22
pencil_brush.class.ts	91.91%	85.42%	100%	93.64%	123–124, 153, 153–155, 277, 281, 286–287, 69–70, 85–86
spray_brush.class.ts	1.16%	0%	0%	1.56%	100–102, 104–105, 113, 113, 113, 113, 113–114, 116–117, 124–125, 127, 129–133, 142, 146–147, 147, 155, 155, 155–158, 160–163, 167–168, 170, 172–175, 178, 185–186, 188, 190–191, 193, 200–201, 203–204, 207, 207, 214, 214, 218, 23–24, 26–28, 28, 28–30, 34, 43, 50, 57, 64, 71, 78, 90–92
src/color
color.class.ts	91.67%	84.51%	100%	94.44%	325–326, 330–331, 334–335, 41, 45, 72–73, 73, 75, 75, 75–76, 78–79
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
index.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
actions.ts	100%	100%	100%	100%
changeWidth.ts	100%	100%	100%	100%
control.class.ts	93.98%	88.89%	90.91%	97.78%	235, 319, 319, 354
controls.render.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
default_controls.ts	86.67%	66.67%	100%	100%	125, 132
drag.ts	100%	100%	100%	100%
rotate.ts	20%	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.ts	94.41%	94.74%	100%	93.59%	129–130, 132–134, 181–183, 42
scaleSkew.ts	80.56%	66.67%	100%	92.31%	14, 28, 30, 30, 30, 32, 34
skew.ts	91.03%	79.31%	100%	97.67%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/filters
2d_backend.class.ts	92%	83.33%	100%	93.75%	35–36
FilterBackend.ts	88.89%	83.33%	100%	90%	11–12
WebGLProbe.ts	40.54%	40%	60%	36.36%	28–30, 30, 30–31, 33–35, 43, 46–48, 48, 48–51, 53, 58
base_filter.class.ts	20.47%	20.41%	31.82%	18%	100–102, 102, 102–103, 110–112, 112, 112–113, 120–123, 123, 123–124, 130, 130, 130–133, 151, 181–186, 190–191, 191, 191–194, 194, 194, 194, 194–196, 202, 211–212, 217–221, 264–267, 276, 287–288, 288, 288–289, 291, 307–309, 309, 309, 309, 309–310, 312, 314–315, 317–318, 320–322, 330–331, 333, 337–339, 343, 343, 343, 347, 347, 347–348, 370, 370, 370–374, 402, 58–59, 81–82, 84, 84, 84–85, 85, 88, 93–95, 97, 97, 97, 97, 97, 97–98
blendcolor_filter.class.ts	6.82%	0%	25%	6.35%	101, 101, 101, 101, 101, 101, 101, 101, 101, 101, 101, 101, 103–106, 108–111, 113–116, 119–122, 124–127, 129–132, 134–137, 139–140, 140, 143–144, 144, 147–148, 148, 151, 153–156, 158–160, 175, 190–195, 60, 76, 80, 90–94, 96–99
blendimage_filter.class.ts	52.86%	70%	9.09%	59.18%	130, 138–139, 154, 170–172, 180, 182, 182, 197–198, 46, 50, 54–58, 62, 72–74
blur_filter.class.ts	2.17%	0%	0%	2.90%	100–101, 103–107, 120, 135–136, 144–146,

asturur · 2022-12-04T20:32:12Z

src/canvas.class.ts

+          this.off('mouse:up', this._mouseUpITextHandler);
+          this._iTextInstances = null;
+          this._hasITextHandlers = false;
+        }


StaticCanvas shouldn't have Itext handlers, if someone come with a real use case we can revert this back

asturur · 2022-12-04T20:35:54Z

@ShaMan123 ready to go for me.
See if you see something meaningful that needs to be changed

ShaMan123

src/util/types.ts:
I saw your comment regarding using instanceof. My suggestion is simple. We create a base class that is exposed only internally for instanceof checks.
So we do something like:

export class TextBase extends FabricObject {

}

export class Text extends TextBase {
   ...logic
}

import { TextBase, type Text } from '..';

function isText(what: FabricObject): what is Text {
   return what instanceof TextBase
}

A bit clumsy IMO. Is it that bad including it (importing the real class)?

ShaMan123 · 2022-12-04T21:13:23Z

src/util/misc/dom.ts

+export const isHTMLCanvas = (
+  canvas: HTMLCanvasElement | string
+): canvas is HTMLCanvasElement => {
+  return !!canvas && (canvas as HTMLCanvasElement).getContext !== undefined;


I wonder if instanceof works on node as well nowadays

ShaMan123 · 2022-12-04T21:13:58Z

src/util/types.ts

+export const isPattern = (filler: TFiller): filler is Pattern => {
+  return (
+    !!filler &&
+    (filler as Pattern).offsetX !== undefined &&


no need for this logic, why not instanceof?

I saw the comments

ShaMan123 · 2022-12-04T21:18:14Z

src/static_canvas.class.ts

+   * The coordinates get updated with @method calcViewportBoundaries.
+   * @memberOf fabric.StaticCanvas.prototype
+   */
+  vptCoords: TCornerPoint;


TCornerPoint: not a great name

yeah i agree.

ShaMan123 · 2022-12-04T21:20:12Z

src/static_canvas.class.ts

+      }
+    });
+
+    if (this._isCurrentlyDrawing) {


this part shouldn't be in StaticCanvas as well, it is canvas domain

i didn't even see it

ShaMan123 · 2022-12-04T21:22:11Z

src/static_canvas.class.ts

+   */
+  drawClipPathOnCanvas(
+    ctx: CanvasRenderingContext2D,
+    clipPath: TCachedFabricObject


nice and simple!

ShaMan123 · 2022-12-04T21:29:55Z

src/static_canvas.class.ts

+              width: this.width / (shouldTransform ? vpt[0] : 1),
+              height: this.height / (shouldTransform ? vpt[3] : 1),
+            };
+          return fill.toSVG(object as Rect, {


I don't like object as Rect
I prefer fixing the types in the Filler

yeah so object needs to be a fabricObject, but it can be FabricObject | StaticCanvas to be honest, it just needs to have TSize or extends the interface of a TSize

So let's change the signature at fill#toSVG

ShaMan123 · 2022-12-04T21:32:52Z

src/static_canvas.class.ts

+      return;
+    }
+    if (isFiller(filler)) {
+      // @ts-ignore TS is so stubbordn that i can't even check if a property exists.


This is really too much, I agree

asturur · 2022-12-05T09:03:37Z

oh ok, i understand what you mean with the empty classes.
A empty shell class that is the base class ONLY for a specific class, in its own file so it doesn't crete any code import.
I can try that when i convert Canvas

….js into TS-convert-static-canvas

asturur · 2022-12-05T09:10:29Z

src/util/types.ts: I saw your comment regarding using instanceof. My suggestion is simple. We create a base class that is exposed only internally for instanceof checks. So we do something like:
export class TextBase extends FabricObject {

}

export class Text extends TextBase {
   ...logic
}
import { TextBase, type Text } from '..';

function isText(what: FabricObject): what is Text {
   return what instanceof TextBase
}
A bit clumsy IMO. Is it that bad including it (importing the real class)?

Well you can't code split anymore.
If you work with canvas and rect, you have to import patterns or text just for the purpose of checking the objects you have aren't Text.
At leat this is my impression.

asturur · 2022-12-05T09:13:27Z

@ShaMan123 Did you reorder imports with some logic?
If prettier doesn't do that don't do it, is that some plugin you have or was willingful?

ShaMan123 · 2022-12-05T09:20:55Z

VSCODE handles organizing imports
ctrl + shift + o
It removes unused, duplicate and reorders

ShaMan123 · 2022-12-05T09:21:25Z

Is there a way for prettier to do that?

ShaMan123 · 2022-12-05T09:25:17Z

A quick search:
https://github.com/trivago/prettier-plugin-sort-imports
https://medium.com/dooboolab/using-eslint-prettier-and-sort-imports-vscode-extensions-for-formatting-open-source-project-16edf317129d => https://marketplace.visualstudio.com/items?itemName=amatiasq.sort-imports

but vscode does that on my machine... not sure if it is core or a plugin

asturur · 2022-12-05T09:52:10Z

is the reorder part that isn't great.
I don't want meaningless commits in the history, when you need to debug something, reading that list trying to figure out if there is a change or a reorder only.
Or if we rename a file then we will have a bit at time changes of ordering all over the place.

ShaMan123 · 2022-12-05T09:56:00Z

is the reorder part that isn't great.

If we make it part of config that won't happen since it will be reordered before committing by vscode/prettier

ShaMan123 · 2022-12-05T11:03:45Z

VSCODE handles organizing imports ctrl + shift + o It removes unused, duplicate and reorders

It is alt+ shift + o on windows

ShaMan123 · 2022-12-06T07:44:06Z

We should move control rendering logic to canvas as well.
Things like

fabric.js/src/static_canvas.class.ts

Line 844 in c0b7c1c

if (this.controlsAboveOverlay && this.interactive) {

ShaMan123 · 2022-12-06T07:56:24Z

I don't understand how the static canvas file passes TS, many fields aren't typed. Did you do some magic config I missed?

ShaMan123 · 2022-12-06T08:05:40Z

I see, it is because of CollectionMixin types, I will try to fix

Co-authored-by: ShaMan123 <shacharnen@gmail.com>

save the salvageable

e2d36f2

asturur added the Work in progress label Dec 3, 2022

asturur added 9 commits December 4, 2022 10:50

some more work

1651970

more work

6af65dc

more work

ce41207

more work

a041f41

some exta work

749de89

some more work

a98d329

some more work

f10f118

finished converting, now lets see how broken it is

915bdd9

ok broken UTs but running

0df5c40

asturur added 5 commits December 4, 2022 16:58

changelog and prettier

93b2c42

some fixes

26a91bf

some fixes

c81b40c

some fixes

e90c20f

all passin

ffdaa88

asturur marked this pull request as ready for review December 4, 2022 20:28

asturur commented Dec 4, 2022

View reviewed changes

asturur removed the Work in progress label Dec 4, 2022

ShaMan123 added 3 commits December 4, 2022 23:36

template string

276ca50

jsdoc

6081074

imports + more jsdoc

33390bc

ShaMan123 previously approved these changes Dec 4, 2022

View reviewed changes

asturur added 2 commits December 5, 2022 10:06

added comment

543b773

Merge branch 'TS-convert-static-canvas' of github.com:fabricjs/fabric…

0335cc5

….js into TS-convert-static-canvas

asturur dismissed ShaMan123’s stale review via 0335cc5 December 5, 2022 09:09

asturur merged commit 4107721 into master Dec 5, 2022

asturur deleted the TS-convert-static-canvas branch December 5, 2022 09:53

frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023

chore(TS): convert StaticCanvas (fabricjs#8485)

44bf7fe

Co-authored-by: ShaMan123 <shacharnen@gmail.com>

ShaMan123 added a commit that referenced this pull request Mar 16, 2024

chore(TS): convert StaticCanvas (#8485)

5640370

Co-authored-by: ShaMan123 <shacharnen@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TS): convert StaticCanvas #8485

chore(TS): convert StaticCanvas #8485

asturur commented Dec 3, 2022 •

edited

github-actions bot commented Dec 4, 2022 •

edited

github-actions bot commented Dec 4, 2022 •

edited

asturur Dec 4, 2022

asturur commented Dec 4, 2022

ShaMan123 left a comment •

edited

ShaMan123 Dec 4, 2022

ShaMan123 Dec 4, 2022

ShaMan123 Dec 4, 2022

ShaMan123 Dec 4, 2022

asturur Dec 5, 2022

ShaMan123 Dec 4, 2022

asturur Dec 5, 2022

ShaMan123 Dec 4, 2022

ShaMan123 Dec 4, 2022

asturur Dec 5, 2022

ShaMan123 Dec 5, 2022

ShaMan123 Dec 4, 2022

asturur commented Dec 5, 2022

asturur commented Dec 5, 2022

asturur commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

asturur commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 6, 2022

ShaMan123 commented Dec 6, 2022

ShaMan123 commented Dec 6, 2022 •

edited

chore(TS): convert StaticCanvas #8485

chore(TS): convert StaticCanvas #8485

Conversation

asturur commented Dec 3, 2022 • edited

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Dec 4, 2022 • edited

github-actions bot commented Dec 4, 2022 • edited

Choose a reason for hiding this comment

asturur commented Dec 4, 2022

ShaMan123 left a comment • edited

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

asturur commented Dec 5, 2022

asturur commented Dec 5, 2022

asturur commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

asturur commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 5, 2022

ShaMan123 commented Dec 6, 2022

ShaMan123 commented Dec 6, 2022

ShaMan123 commented Dec 6, 2022 • edited

asturur commented Dec 3, 2022 •

edited

github-actions bot commented Dec 4, 2022 •

edited

github-actions bot commented Dec 4, 2022 •

edited

ShaMan123 left a comment •

edited

ShaMan123 commented Dec 6, 2022 •

edited