patch(): node build follow up #8652

ShaMan123 · 2023-02-04T00:35:01Z

Motivation

Follow up #8632

Description

The dirty part of #8632

Changes

cleanUpJsdomNode => getEnv().dispose
getEnv().window => getWindow (done for ease of coding, finding etc. and one day perhaps when we split browser logic we can use window directly since there really is no use for it in node)
getEnv.document => getDocument (same)
refactor WebGLProbe and expose NodeGLProbe (a stub) both under GLProbes, now part of env
remove isLikelyNode: tests use isNode() defined in config files
remove setEnvForTests => use setEnv directly

Gist

In Action

https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame#browser_compatibility jsdom: jsdom/jsdom@8a6894c

github-actions · 2023-02-04T00:38:05Z

Coverage after merging node-build-1 into master will be

83.10%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
fabric.ts	100%	100%	100%	100%
index.node.ts	7.69%	100%	0%	14.29%	18, 21, 24, 30, 33, 36
src
Collection.ts	95.65%	95%	89.66%	97.83%	205–206, 231–232
CommonMethods.ts	100%	100%	100%	100%
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	138–139, 164–165, 39–40, 48, 57, 91, 99
Pattern.ts	95.77%	94.29%	100%	96.43%	124, 135, 144
Point.ts	100%	100%	100%	100%
Shadow.ts	98.48%	96%	100%	100%	156
cache.ts	97.06%	90%	100%	100%	56
config.ts	75%	64.29%	66.67%	82.76%	138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
typedefs.ts	100%	100%	100%	100%
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	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.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%	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.ts	78.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.ts	93.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.ts	95.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.ts	100%	100%	100%	100%
src/color
Color.ts	92.16%	86.49%	100%	94.29%	330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
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%	235, 319, 319, 354
changeWidth.ts	100%	100%	100%	100%
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%	122, 129
drag.ts	100%	100%	100%	100%
polyControl.ts	6.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.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.49%	92.77%	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	93.75%	50%	100%	100%	20
node.ts	76.92%	50%	66.67%	88.24%	27, 31–32, 32, 32
src/filters
BaseFilter.ts	19.77%	20%	30%	17.65%	100–101, 101, 101–102, 109–112, 112, 112–113, 119, 119,

ShaMan123

cleanUpJsdomNode => getEnv().dispose
getEnv().window => getWindow
getEnv.document => getDocument
refactor WebGLProbe and expose NodeGLProbe (a stub) on env
remove isLikelyNode: tests use isNode()
remove setEnvForTests => use setEnv directly

src/parser/parseSVGDocument.ts

src/util/animation/AnimationFrameProvider.ts

github-actions · 2023-02-04T01:08:48Z

Build Stats

file / KB (diff)	bundled	minified
fabric	900.398 (-0.909)	305.244 (-0.627)

ShaMan123 · 2023-02-04T01:10:41Z

src/env/node.ts

@@ -20,31 +20,37 @@ const { window: virtualWindow } = new JSDOM(
      FetchExternalResources: ['img'],


I don't think this is needed, it seems to be a relic of the past

it could still be needed to actually load base64 images.
Since canva is not mandatory for jsdom, loading images is completely optional.
That is my understanding, if you want them to actually be loaded, you have to ask for it

I think it is dead.
Not typed and I searched the repo and couldn't find it.

yes is not more there indeed.
It is replaced by resources 'usable' we have below.

ShaMan123

Good thing I held back and waited with all this
A lot of ink for very little change, basically a no brainer, a cleanup task

asturur · 2023-02-04T21:55:37Z

src/env/index.ts

-export const setEnvForTests = (window: Window) => {
-  env.document = window.document;
-  env.window = window;
-};


this was specific for tests, for now setEnv is a necessity of delaying an import / definition issue, and i don't think it is a feature yet. I would like this to stay setEnvForTests ( and even that was some kind of issue of testing and not a real need ) and setEnv to not be exposed

I was thinking we should expose setEnv for devs who work on a strange env, someone wrote about it in #8208

in case we will have specific pr with new feature for it after evaluating which are the side effects.

src/env/index.ts

src/env/node.ts

asturur · 2023-02-04T22:15:48Z

src/filters/GLProbes/WebGLProbe.ts

-    if (getEnv().isLikelyNode) {
-      return;
-    }
+  queryGL() {


why renaming WebGL to GL?
queryWebGL is meant to detect support for WEBGL, i think the old name is good.
GLPrecision change i think is ok, since the constant is stil GL_PRECISION_XXXXX in the WEBGL api.

because I saw there is support for GL on node canvas...
so a dev could somehow wire it up with fabric and use a NodeGLProbe

Anyways since we need a probe for node I had to rename the base class/folder

The npm module is called gl but is still the WebGL specs api.

asturur · 2023-02-04T22:34:28Z

Seems all good to me, apart some naming issues.
For me setEnv vs setEnvForTests is imporant.
setEnv is not a feature for now, even Env was kind of an experiment itself for the lack of not knowing what to do with fabric object and HEADER.js file.
So let's not expose it outside the context of tests.

ShaMan123 · 2023-02-04T23:40:09Z

For me setEnv vs setEnvForTests is imporant.
setEnv is not a feature for now, even Env was kind of an experiment itself for the lack of not knowing what to do with fabric object and HEADER.js file.
So let's not expose it outside the context of tests.

How?

ShaMan123

Remaining

GL naming
setEnv for tests

apart that is good to go
You can change as you see fit and merge

asturur · 2023-02-05T08:45:58Z

This PR introduces a circular dependency.
For rollup is just a warning, but we need to find a way to block build on those.
Unbundled code wouldn't work with a circular dependency

ShaMan123 · 2023-02-05T08:46:19Z

let me look

ShaMan123 · 2023-02-05T08:50:44Z

test/node_test_setup.js

@@ -82,7 +82,7 @@ var virtualWindow = new jsdom.JSDOM(
    resources: new CustomResourceLoader(),


Do we really need this?
Is just a console log...
And then we don't need to reconfigure the env so we won't need to expose setEnvForTests as well

we won't need to expose it in fabricJS, nor remove it, we will probably be able to import it directly from the build folder.

I mean we don't need this configuration

The CustomResourceLoader justs log to console a line
It is meaningless

So we can use the node env from fabric w/o this headache

ShaMan123 · 2023-02-05T08:53:07Z

Regarding the import cycle
Truth is we can simply import the browser env directly from the WebGLProbe since it runs only on browser env

asturur · 2023-02-05T08:54:59Z

yes is not an issue right now we just need a way to detect them properly

ShaMan123 · 2023-02-05T08:55:20Z

OR we can make initFilterBackend async and do a dynamic import of the GLProbe depending on getEnv.
This will allow us to remove the probe from the env

ShaMan123 · 2023-02-05T08:58:23Z

Regarding the import cycle Truth is we can simply import the browser env directly from the WebGLProbe since it runs only on browser env

That is wrong! Since then we init the browser env at import time

ShaMan123 · 2023-02-05T09:01:19Z

We can safely use document though

asturur · 2023-02-05T09:01:42Z

we will find a solution, the unbundle import is just a playground now, and is node only, so not an issue.

…c env dispose ( instead of cleanup JSDOM ) (#8652)

ShaMan123 added 14 commits February 3, 2023 23:30

Update index.ts

3918cdc

env => getWindow, getDocument

bdf3ec5

remove polyfills

aba8e2e

https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame#browser_compatibility jsdom: jsdom/jsdom@8a6894c

cleanUpJsdomNode => env#dispose

2c647a1

node isTouchSupported

dfa7435

Update node.ts

51a6bad

cleanup

27d9761

refactor gl probe

d508509

remove polyfill

762671f

dump isLikelyNode

8c1b6f2

Update ITextKeyBehavior.ts

5767f8c

dep(): setEnvForTests, isNode, fix anim frame provider

ed18297

tests

e02046b

Update index.node.ts

384f8a5

ShaMan123 added 4 commits February 4, 2023 02:45

Update index.ts

f720c72

Update node.ts

c8982ab

Update dom_misc.ts

b5805e9

Update env.js

df6fca8

ShaMan123 commented Feb 4, 2023

View reviewed changes

src/parser/parseSVGDocument.ts Show resolved Hide resolved

src/util/animation/AnimationFrameProvider.ts Show resolved Hide resolved

Update CHANGELOG.md

ac19649

ShaMan123 requested a review from asturur February 4, 2023 01:04

Update build.yml

be49fc4

ShaMan123 commented Feb 4, 2023

View reviewed changes

ShaMan123 added the v6 label Feb 4, 2023

ShaMan123 commented Feb 4, 2023

View reviewed changes

asturur reviewed Feb 4, 2023

View reviewed changes

src/env/index.ts Outdated Show resolved Hide resolved

asturur reviewed Feb 4, 2023

View reviewed changes

src/env/node.ts Outdated Show resolved Hide resolved

asturur reviewed Feb 4, 2023

View reviewed changes

ShaMan123 added 3 commits February 5, 2023 01:41

rename

b87b1c3

Update index.ts

8b18c6f

Merge branch 'master' into node-build-1

92b1657

ShaMan123 requested a review from asturur February 4, 2023 23:57

ShaMan123 commented Feb 5, 2023

View reviewed changes

asturur added 4 commits February 5, 2023 09:08

removed old JDOM option

eaee5f4

resovle TS warnings

8eab8d1

restored setEnvForTests

cc9ed6c

more specific changelog

18a5e6b

ShaMan123 commented Feb 5, 2023

View reviewed changes

asturur approved these changes Feb 5, 2023

View reviewed changes

asturur merged commit fb9b2b6 into master Feb 5, 2023

ShaMan123 deleted the node-build-1 branch February 5, 2023 09:03

ShaMan123 mentioned this pull request Feb 24, 2023

[Bug]: FabricJS does not work with NextJS 13 and the new appDir #8444

Closed

7 tasks

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

chore(): Added WebGLProbe to env, removed isLikelyNode, added specifi…

4c9368b

…c env dispose ( instead of cleanup JSDOM ) (#8652)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

patch(): node build follow up #8652

patch(): node build follow up #8652

ShaMan123 commented Feb 4, 2023 •

edited

Loading

github-actions bot commented Feb 4, 2023 •

edited

Loading

ShaMan123 left a comment

github-actions bot commented Feb 4, 2023 •

edited

Loading

ShaMan123 Feb 4, 2023

asturur Feb 4, 2023

ShaMan123 Feb 4, 2023

asturur Feb 5, 2023

ShaMan123 left a comment •

edited

Loading

asturur Feb 4, 2023

ShaMan123 Feb 4, 2023

asturur Feb 5, 2023

asturur Feb 4, 2023

ShaMan123 Feb 4, 2023

ShaMan123 Feb 4, 2023

asturur Feb 5, 2023

asturur commented Feb 4, 2023

ShaMan123 commented Feb 4, 2023

ShaMan123 left a comment •

edited by asturur

Loading

asturur commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

ShaMan123 Feb 5, 2023

asturur Feb 5, 2023

ShaMan123 Feb 5, 2023

ShaMan123 Feb 5, 2023 •

edited

Loading

ShaMan123 Feb 5, 2023

ShaMan123 commented Feb 5, 2023

asturur commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

asturur commented Feb 5, 2023

		@@ -20,31 +20,37 @@ const { window: virtualWindow } = new JSDOM(
		FetchExternalResources: ['img'],

		@@ -82,7 +82,7 @@ var virtualWindow = new jsdom.JSDOM(
		resources: new CustomResourceLoader(),

patch(): node build follow up #8652

patch(): node build follow up #8652

Conversation

ShaMan123 commented Feb 4, 2023 • edited Loading

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Feb 4, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2023 • 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

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 Feb 4, 2023

ShaMan123 commented Feb 4, 2023

ShaMan123 left a comment • edited by asturur Loading

Choose a reason for hiding this comment

asturur commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Feb 5, 2023

asturur commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

ShaMan123 commented Feb 5, 2023

asturur commented Feb 5, 2023

ShaMan123 commented Feb 4, 2023 •

edited

Loading

github-actions bot commented Feb 4, 2023 •

edited

Loading

github-actions bot commented Feb 4, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 left a comment •

edited by asturur

Loading

ShaMan123 Feb 5, 2023 •

edited

Loading