fix(Canvas): sync cleanup of dom elements in dispose

[ShaMan123]

Motivation

closes #8901
closes #8899

Description

Changes

• expose cleanupDOM on canvas that runs from dispoe (sync)
• moved dispose to node env and use only in index.node

Gist

In Action

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	900.794 (+0.194)	304.075 (+0.114)

[ShaMan123]

• expose cleanupDOM on canvas that runs from dispoe (sync)
• moved dispose to node env and use only in index.node

[github-actions]

Coverage after merging dispose-dom-sync into master will be

83.66%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	47.83%	100%	25%	60%	17, 20, 23, 40, 43, 46
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.21%	91.89%	90%	93.33%	116, 127, 136, 29, 92
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	107, 107, 107, 109, 111, 113–115, 117–120, 127–128, 135, 137, 22–23, 31–35, 39–43, 50–53, 61–65, 67, 75, 75, 75, 75, 75–76, 78, 78, 78–81, 83, 91–92, 94, 96–98
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%	106, 106, 106, 106, 106–107, 109–110, 117–118, 120, 122–126, 135, 139–140, 140, 148, 148, 148–151, 153–156, 16, 160–161, 163, 165–168, 17, 171, 178–179, 181, 183–184, 186, 19, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 21, 21, 211, 22–23, 27, 36, 43, 50, 57, 64, 83–85, 93–95, 97–98
src/canvas
Canvas.ts	78.87%	77.54%	81.67%	79.41%	1001–1002, 1002, 1002–1004, 1006–1007, 1007, 1007, 1009, 1017, 1017, 1017–1019, 1019, 1019, 1025–1026, 1034–1035, 1035, 1035–1036, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1198–1200, 1203–1204, 1277, 1396, 1519, 1589, 162, 187, 297–298, 301–305, 310, 333–334, 339–344, 364, 364, 364–365, 365, 365–366, 37, 374, 379–380, 380, 380–381, 383, 392, 398–399, 399, 399, 41, 442, 450, 454, 454, 454–455, 457, 539–540, 540, 540–541, 547, 547, 547–549, 569, 571, 571, 571–572, 572, 572, 575, 575, 575–576, 579, 588–589, 591–592, 594, 594–595, 597–598, 610–611, 611, 611–612, 614–619, 625, 632, 669, 669, 669, 671, 673–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 742, 742, 800–801, 801, 801–802, 804, 807–808, 808, 808–809, 811–812, 815, 815–817, 820–821, 891, 903, 910, 931, 963, 984–985
SelectableCanvas.ts	94.39%	91.16%	94.64%	96.62%	1119, 1119–1120, 1123, 1143, 1143, 1201, 1254–1255, 1276, 1284, 1409, 1411, 1413–1414, 518, 698–699, 701–702, 702, 702, 751–752, 813–814, 867–869, 901, 906–907, 934–935
StaticCanvas.ts	96.86%	92.91%	100%	98.61%	1102–1103, 1103, 1103–1104, 1224, 1234, 1288–1289, 1292, 1327–1328, 1405, 1414, 1414, 1418, 1418, 1465–1466, 310–311, 328, 759, 771–772
TextEditingManager.ts	100%	100%	100%	100%
src/color
Color.ts	92.16%	86.49%	100%	94.29%	314–315, 319–320, 323–324, 42, 72–73, 73, 75, 75, 75–76, 78–79
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
Control.ts	93.90%	88.89%	90.91%	97.73%	232, 319, 319, 354
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%	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/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%	27, 31–32, 32, 32, 37
src/filters
BaseFilter.ts	22.04%	23.21%	32%	19.05%	100, 100, 100–101, 108–111, 111, 111–112, 118, 118, 118–121, 139, 157, 171–176, 180–181<

[ShaMan123]

Shouldn't we dispose of all cache canvases in Object#dispose??

[ShaMan123]

ts warning

[ShaMan123]

I think this ok
It is better than #8901 for the case of running an async toCanvasElement during disposing but I don't like the jsdom dispose thing

[ShaMan123]

added disposing to object...
I have to say I don't like it and prefer to have that specific dispose logic in the node entry only like I did with image before I reverted.
But it means we need to apply a mixin to fabricObject to patch destroy

[ShaMan123]

But I can live with it.
It is strange now that canvas disposing is done in the node entry file though.
But my point for doing that is say one day we use the env dispose in the browser as well or a different env that reuses the element. We cleanup the dom, lets say hot relaoding occured so everything is great. But then destroy kicks in a cripples the element and no one know what happened.
That is why I left it in the node entry. I would feel better if it were part of the dom cleanup function but then the pending rendering will throw so we missed out the entire point of this effort.
All this is why I am not convinced this is better than #8901 that says - once you called dispose don't expect a pending render to succeed.
So all in all I prefer #8901 and wish do better understand why you don't

[ShaMan123]

Well, yes. Thinking of it I dislike this.

[ShaMan123]

I would argue that we should combine the 2.
Have dispose be sync and expose the cleanupDOM method

[ShaMan123]

just did that in #8901

[asturur]

i don't like the idea of try catching in a render because regardless of typescript requiring us a context, then we know there are situation in which will fail anyway because somethin could fire a render after dispose.
requestAnimationFrame is async, it doesn't cancel imperatively as we would like to, we have to handle it asyncronously.
There is no reason to force it to be sync after all the efforts we did to cancel it properly.

On top of that again, the issue here is not to be sync or async, is the interaction with react and we need a foolproof react wrapper at some point, and cleaning up the dom elements is just a part of it.

Probably ( didn't test ) you don't even need to dispoe the canvas on hot reload, you can even just call cleanupDOM and let the instance be garbage collected naturally.

[asturur]

Is also important that before reverting and changing idea on something we think twice or three times.
We can't waste hours on a pr and the at the first issue remove it without thinking of other ways to solve the issue.
this approach just change the order of operations and fixes the issue.

[asturur]

Please give an explanation why you dislike this approach and then close the pr.
You don't have to do things you don't like

[ShaMan123]

I saw you commented on garbage collection.
Why then not leave all of the disposing part except the dom cleanup to gc and remove this logic from fabric altogether?
A different approach can be throwing when dispose is called during a requested/active render - That is better IMO because it forces the dev to own the cleanup of animations, requested renders, etc. and it can be easily caught by the dev with try-catch

[ShaMan123]

What do you think?
Is disposing is a node only thing?
We can merge this and reiterate if needed.

What I have against it:
If we instruct users to use cleanupDOM and not dispose, then why have the dispose logic?
If dispose meant for node only?

[asturur]

I saw you commented on garbage collection. Why then not leave all of the disposing part except the dom cleanup to gc and remove this logic from fabric altogether? A different approach can be throwing when dispose is called during a requested/active render - That is better IMO because it forces the dev to own the cleanup of animations, requested renders, etc. and it can be easily caught by the dev with try-catch

A one issue that we got in the past is that node was accumulating canvases buffer till would give an exception.
Node GC would eventually clear up the JS part of node-canvas and JSDOM but somehow those buffers needed to be removed manually or node would create issues.
There are issues in the repo on this matter and unless we can prove that a correct code initialization and disposal will prevent any memory leak, i would leave the code in place at least to say, we are trying t make your server not die.

[ShaMan123]

I would expect that to be fixed by node-canvas
But yeah, if we can prevent a melt down with a line of code we should