fix(BaseConfiguration): set devicePixelRatio from window

[AdamMerrifield]

Motivation

closes #9464

Description

In the browser window.devicePixelRatio is not set to the config object until the first run of getEnv. This needs to run only on the browser instance of fabric.

Changes

Add a getEnv() call to index.ts so it runs a single time in the browser to set config.devicePixelRatio.

In Action

https://jsfiddle.net/35Lxfq7v/1/

[ShaMan123]

This will break SSR I believe

[ShaMan123]

the question is why getEnv isn't called when the app starts

[AdamMerrifield]

It looks like you're right, this does break SSR. I am not sure how to call this at app start while allowing SSR to continue to function other than wrapping this in a conditional of typeof window !== 'undefined'.

Calling getEnv inside the constructor of StaticCanvas seems to work with the SSR codesandbox example, but is not eloquent.

[ShaMan123]

It looks like you're right, this does break SSR. I am not sure how to call this at app start while allowing SSR to continue to function other than wrapping this in a conditional of typeof window !== 'undefined'.

that is also breaking SSR
please find why it is not called, or rather what calls it for the second StaticCanvas or for the first Canvas

[AdamMerrifield]

In the example above this is the trace for getEnv's first run which is during canvas1.setDimensions

The first use of config.devicePixelRatio is from _setDimensionsImpl in the constructor for StaticCanvas.
If you do not run setDimensions on either canvas, both are blurry https://jsfiddle.net/rcoj3pz6/1/

[asturur]

I see 2 issues here:

1. calling getEnv calls a config.configure that is unexpected, because i m getting something and i do not know i m about to execute a side effect that could potentially override my config.
2. somehow when a canvas starts or when a dom manager starts we need to grab the config and understand if a custom value for device pixel ratio has been added, if not, we need to add the default.

Alternatively we need a custom node config class that has retinaScaling set to 1 and the browser version that default to window.devicepixelRatio.

The config module could also just have a typeof window !== 'undefined' ? window.devicePixelRatio : 1 as the default value for the retina scaling?

[ShaMan123]

Not the correct approach

[ShaMan123]

No value can be set before loading or else SSR will break

[asturur]

can you explain this a little bit better?

No value can be set before loading or else SSR will break

What are the tech constrain of SSR in your opinion and which are the one that we add on our own to make things easier for the devs

[ShaMan123]

fabric logic can't execute until the page loads in the client for SSR to work
This means that calculating stuff as const in the global scope when importing is not possible and that is why we exposed the config object and that is why getEnv defaults to the browser env when it is not set. It can't be set at import time because of SSR. The window object is not defined at that point.
I did not investigate why StaticCanvas does not call setEnv. That is the bug. The solution is not to call it, but to understand first why it is not called.
Also the fact that the second canvas is correct points to that direction or else I would expect all canvases to be blurry because of not initializing env

[ShaMan123]

I think I should add an e2e SSR test, @asturur thoughts?

[ShaMan123]

I see 2 issues here:

1. calling getEnv calls a config.configure that is unexpected, because i m getting something and i do not know i m about to execute a side effect that could potentially override my config.
2. somehow when a canvas starts or when a dom manager starts we need to grab the config and understand if a custom value for device pixel ratio has been added, if not, we need to add the default.

Alternatively we need a custom node config class that has retinaScaling set to 1 and the browser version that default to window.devicepixelRatio.

The config module could also just have a typeof window !== 'undefined' ? window.devicePixelRatio : 1 as the default value for the retina scaling?

This is how we solved the SSR issue (typeof window !== 'undefined' ? window.devicePixelRatio : 1 will always equal 1 in SSR)
If we need to support configuring dpr we can simply add a check to see if it is defined and if not define it.

[asturur]

@ShaMan123 for me the code as i see it at commit 61ba553 is the way i would have solved it.
Solve the side effect and in ssr gives you 1.

I agree the only issue we have to avoid is that importing fabric breaks the code in node.
I do think is still on developer knowing that it can't run fabricJS code outside event handlers or useEffect(s) otherwise SSR will break.

If the developer needs to use SSR to produce an SVG to add to the dom at rendering time ( i don't know to send a placeholder of the canvas that looks like the canvas and then swap it to interactive ) that can be done, importing the node version of fabric.

[ShaMan123]

If you do that you can't import fabric outside of ueEffect and you have to use dynamic imports
Please let me take a look at this before moving forward.
I still did not understand why the method is not called as it should, we are debating the wrong thing

[asturur]

@ShaMan123 why so?

having a typeof window !== undefined in node code is totally fine imho. It doesn't break anything.

On top of that the fact that we are calling a setConfig without dev consent, is a bit strange.
We could be overriding something already set.

[ShaMan123]

can you explain why we should support changing the dpr apart from the dpr change event that we should support out of the box?

[ShaMan123]

Sticking to the bug report, since changing StaticCanvas to Canvas fixes the bug it signals that the bug is not here.
Yes, it can be patched by endless changes but they will destroy SSR support that I worked hard to achieve

[ShaMan123]

having a typeof window !== undefined in node code is totally fine imho. It doesn't break anything.

That is wrong. It is breaking. Will SSR run this line on import? If so it is breaking

[ShaMan123]

I asked to trace the call for a reason
I found the bug and it was not hard to find so adhearing my advice would have resoled this long before.
_isRetinaScaling accesses config.devicePixelRatio before getEnv is called.
Simple as that.
I will PR a fix

[ShaMan123]

I still want to undersatnd why dpr is configurable
I see no reason for that apart for v5 not supporting imports

[ShaMan123]

To patch this you can call getEnv() from _isRetinaScaling before it accesses the config

[asturur]

what do you mean by dpr?

[ShaMan123]

DevicePixelRatio

[asturur]

@AdamMerrifield Sorry we got in a convoluted discussion over this PR, this PR was OK and we are going to merge it and then reiterate on additional improvements.
But this fixes the bug you found and should be merged

We discussed this largely, this is a fix, we can follow up with concerns and larger changes