Fix typing for DOMParser

[tushuhei]

Motivation

Related to #8869.
Creating a DOMParser from the window object within the parser module leads to a TypeScript error, which entails any casting. This PR proposes encapsulating the DOMParser instantiation in the env script to make it work isomorphically, so that loadSVGFromString can simply call getDOMParser().parseFromString.

[ShaMan123]

This is a good concept IMO.
The only thing is it should reference the factory instead of instantiating an instance of DOMParser for these reasons:

• loading time
• most apps won't use it
• SSR env is broken this way:
  

Also please:

• replace all usages in fabric code (find and replace should do the trick)
• add a changelog entry

[ShaMan123]

Great work, thanks!

Pushed a test and a minor change

@asturur I am merging unless you have objections

[ShaMan123]

This is good because it takes fabric a step further to move away from JSDOM!

[tushuhei]

Yay, thanks for reviewing!

[asturur]

I have questions.
The code was isomorphic before already since the place we swapped is in a single place.
Are we trying to solve a typing issue or something else?

[asturur]

This is good because it takes fabric a step further to move away from JSDOM!

i want to be clear that i don't think that should be direction at all, as of today it isn't. JSDOM was introduced exactly to avoid maintainin 2 paths of code. If you remove JSDOM you have to grow getEnv and the understanding and handling of the dom that i don't want to. FabricJS draws things and it doesn't want to solve isomorphim issues between node and browser, is just a field i don't want to get into.

[tushuhei]

My intention for this PR is to solve the typing issue for #8869

[ShaMan123]

This is good because it takes fabric a step further to move away from JSDOM!

i want to be clear that i don't think that should be direction at all, as of today it isn't. JSDOM was introduced exactly to avoid maintainin 2 paths of code. If you remove JSDOM you have to grow getEnv and the understanding and handling of the dom that i don't want to. FabricJS draws things and it doesn't want to solve isomorphim issues between node and browser, is just a field i don't want to get into.

I will explain my point.
The Offscreen POC made me think.
JSDOM is used for window and document mocks that are mostly unnecessary for non browser env.

• styling HTML elements and wrapping main canvas
• adding event listeners
• using upper canvas
• text editing
• controls
• all interaction logic in general

All are not needed in node but were made possible for the sake of a quick and simple solution (and to allow devs to do stranger things) and because it was too complex to break up the logic (it is not the case anymore, we achieved most of it already).
So instead of extracting browser specific logic to the browser entry point we use JSDOM.

However, this solution might not suffice to make fabric work offscreen, and I want it to. Perf gains are too crazy not to. So many perf issues can be resolved simply by rendering things offscreen beforehand, stuff that we discussed, e.g. rendering special cases such as scaling, zooming and panning, even filtering.
Take scaling an object for example. Fabric (or the dev) could render the selected object in 3 scale levels offscreen once it is selected, so when the user is scaling the object, resolution will be great w/o lagging the scale interaction (the scale interaction will switch the canvas that it uses to match the scale level).
Same can be said about caching in general. The list is long.
I am not saying fabric should do that. I am saying it is such an important perf gain that we shouldn't disregard it.

What is needed that is not already included in env or in this PR that JSDOM gives:

• create canvas, image elements

I couldn't think of anything else.

Offscreen doesn't have a window/document concept as well and can't mutate the dom and is by most means the same as node in terms of how you would use it I guess, just for rendering, not for interaction. Loading JSDOM into it sounds silly to me. I am not sure how that would work. It must use OffscreenCanvas. Also, creating an image element is different.

Parsing is a good candidate for offscreen handling. This is how this topic is related to this PR. So having it in env is good IMO.

That is my point. But it is not strictly related to this PR.

[asturur]

The SVG code uses a lot of the DOM code that JSDOM polyfills.

[ShaMan123]

Good point.
So should we try to make JSDOM work offscreen and somehow configure it to use Offscreen canvas?
Or should parsing be done on the main thread?

[asturur]

@tushuhei for me the type isse should be solved in this way:

diff --git a/src/env/index.ts b/src/env/index.ts
index dcaa6a5a9..5e88f87c4 100644
--- a/src/env/index.ts
+++ b/src/env/index.ts
@@ -21,4 +21,5 @@ export const getEnv = () => env || getBrowserEnv();

 export const getDocument = (): Document => getEnv().document;

-export const getWindow = (): Window | DOMWindow => getEnv().window;
+export const getWindow = (): (Window & typeof globalThis) | DOMWindow =>
+  getEnv().window;
diff --git a/src/env/types.ts b/src/env/types.ts
index b0c09c0ec..110cb2780 100644
--- a/src/env/types.ts
+++ b/src/env/types.ts
@@ -7,7 +7,7 @@ export type TCopyPasteData = {
 };
 export type TFabricEnv = {
   document: Document;
-  window: Window | DOMWindow;
+  window: (Window & typeof globalThis) | DOMWindow;
   isTouchSupported: boolean;
   WebGLProbe: GLProbe;
   dispose(element: Element): void;

And i m not saying that because i don't want the new function in the env, but because if we have a type issue we should be able to solve it with types and not adding new code.
Is silly to me that TS knows that we are returning window and doesn't know that DOMParser is on window. And it doesn't know becase when you use the global var window ts identify it as window & typeof globalthis. Since i m not an expert in TS for me is very important to understand what is the proper way to solve those kind of issue.

What is your thought?
Which is the correct way to explain TS that on window you can find all the global variables?

[ShaMan123]

To me looks a decent solution as well

[tushuhei]

@asturur I agree with your suggested change. I updated the code accordingly.