fix(useStyles): manageSheet must be in the same effect as unmanageSheet

[enisdenjo]

1608 fixed the failing tests caused by #1604, but doing so reintroduced the original bug breaking React 18 support...

manageSheet must be in a separate useEffect because it can run more times than useMemo - like during fast-refresh or running in strict mode. Since manageSheet is currently in the useMemo, unmanageSheet from useEffect gets called more times than manageSheet, unmanaging the sheet while in use.

All renderToString tests broke with #1604 because useEffects don't run during SSR. The fix is included here; however, your tests are wrongly set up since isInBrowser will return true when using renderToString.

@kof I don't want to tingle with your tests setup, can you please make sure that isInBrowser is false when testing with renderToString? Doing so will pass all the tests.

[kof]

The problem is that we are actually running those in the browser, so I can't make isInBrowser false without breaking it's semantics. What else can we do?

[kof]

I guess the tests which rely on being on the server simply should not run in the browser, we need to move them

[kof]

Alternative is that semantics should not rely on running in the browser or server, but on rendering using renderToString vs mounting to the dom.

[kof]

Technically the fact we run in the browser in this case is not that important, the react API we use is what's important, right?

[enisdenjo]

I guess the tests which rely on being on the server simply should not run in the browser, we need to move them

Exactly.

Technically the fact we run in the browser in this case is not that important, the react API we use is what's important, right?

Sheet and dynamic rules management is now correct in terms of React API usage. Also with https://github.com/cssinjs/jss/pull/1609/commits/21f87aeafee581c08d11b4797e238924b8a04672, both server and client renders will manage the sheet as expected.

The only problem now is that all SSR tests are false negative, and to fix them - isInBrowser has to be coherent to its actual environment.

[enisdenjo]

P.S. I am already running this patch in a prod env of mine, regressions from #1608 are gone and everything is working as expected.

[kof]

The only problem now is that all SSR tests are false negative, and to fix them - isInBrowser has to be coherent to its actual environment.

Yeah, but should we run tests without browser or should we add some kind of hint to the logic to let it know what API are we using, so that it doesn't rely on isInBrowser?

[kof]

Technically its a valid case to call renderToString in the browser. There are for sure use cases to do so.

[kof]

So probably we need to change the flag we rely on to a different semantics and make sure it has correct value depending on the react API being used?

[enisdenjo]

Technically its a valid case to call renderToString in the browser. There are for sure use cases to do so.

Sure, was just thinking through usual use cases.

So probably we need to change the flag we rely on to a different semantics and make sure it has correct value depending on the react API being used?

If we go down that way, we'd need another flag, something like isSSR. The question then is - who sets this flag? Users using renderToString in the browser will have to:

global.isSSR = true;
renderToString(<MyComponentWithJSS />);

Googling "how to detect SSR", all the answers are window == undefined. I worry that you guys might overcomplicate it thinking about edge cases - like doing SSR in browsers.

[kof]

Just researched a bit, indeed react doesn't have any good way to see a difference, they are relying themselves on the dom existence.

In that case we should be basing the code that users use on a foundation that renderToString and alike is used on the server with no global document variable and if you really wanted to do that on the client, you would have to set some flag that would let you do that. Same flag can be used for tests.

[kof]

So yeah, some kind of isSSR option on JssProvider would do the trick

[enisdenjo]

So yeah, some kind of isSSR option on JssProvider would do the trick

That would work! The isSSR value simply defaults to isInBrowser behaviour and you can supply whatever you want still.

Furthermore, this would fix the test issues without too much refactoring - just <JssProvider isSSR> wherever renderToString.

[enisdenjo]

Any news? What are the next steps?

[kof]

• whenever renderToString in the browser env, otherwise shouldn't need it

Next steps: lets add it in this PR?

[enisdenjo]

whenever renderToString in the browser env, otherwise shouldn't need it

Huh? I don't understand the sentence. Are we proceeding with isSSR in JssProvider defaulting to the isInBrowser check?

If so, I recon it would be much better as a separate PR, this one has nothing to do with SSRs.

[enisdenjo]

@kof I hope I understood well, see #1610. Once it lands, I'll rebase this PR on it and all tests will pass. Please take a look.

[kof]

Merged #1610

[enisdenjo]

@kof master is merged. This is now ready.

[kof]

great, thank you!

[enisdenjo]

Great stuff, thanks! Would be awesome if you could release the alpha.1 so that I can test this out with the bundled package - no pressure though, my fixes are already live anyways.

[kof]

released in 10.9.1-alpha.1