Fix sheets not rendered during SSR when using hooks API

[felthy]

Stylesheets that use useStyles() are omitted when when calling sheetsRegistry.toString() during server side rendering.

By comparing createUseStyles() with the withStyles() HoC API (which does not exhibit this problem) I noticed that the HoC component calls manageSheet() from its constructor, whereas useStyles() calls it from inside useEffectOrLayoutEffect(), i.e. after the component mounts. That means it won't be called at all on the server.

I've moved the manageSheet() call to be together with the addDynamicRules() and getSheetClasses() calls in the useReducer() initializer function, in the same order as they are called in the WithStyles constructor. This also means I can omit the call to context.registry.add(sheet).

This has fixed the SSR problem for me. However I currently have some failing tests. I’m creating the PR early though because it’ll probably take me a while to figure out what the tests are testing and why they’re failing.

[HenriBeck]

This is not the correct solution to the problem.

We had a lengthy discussion in the hooks PR about this. It basically boils down to this:

On the server, we want to add the sheet to the registry, but don't want to necessarily "manage" it meaning attaching it to the DOM.

On the client, we need to update the managing of the sheet into a hook so after an update it's correctly managed. On the client, the sheet managing simply needs to happen inside the layout effect.

[felthy]

Sorry about that @HenriBeck I didn't see the discussion on the other PR. Is anyone working on SSR + hooks at the moment? Asked on gitter but I decided to have a go at this myself anyway...

[kof]

Can you pls create a failing test? We will figure out the right fix.

[felthy]

OK, I'll try to do that.

In the meantime, the reason that SheetsRegistry.toString() omits the sheet is that .filter(sheet => sheet.attached) omits it. So that's how I came to think that manageSheet() is required - as well as the fact that that is exactly what withStyles() does (the WithStyles HoC component constructor calls WithStyles.manage at src/withStyles.js#L116).

[HenriBeck]

In the meantime, the reason that SheetsRegistry.toString() omits the sheet is that .filter(sheet => sheet.attached) omits it. So that's how I came to think that manageSheet() is required - as well as the fact that that is exactly what withStyles() does (the WithStyles HoC component constructor calls WithStyles.manage at src/withStyles.js#L116).

Yeah, that's the issue. We either need to drop this check for attached or attach it in the initial state which I'm personally not a fan.

[felthy]

I've reverted my previous commit and added tests that demonstrate the issue: the first test uses the HOC and passes; the second uses the hook and fails.

[kof]

Thanks, I am going to look into it soon.

[felthy]

Terrific, thanks @kof.

@HenriBeck I do agree regarding "managing"/"attaching" on the server, but I also think that both that point, and the other SSR-related discussion in #1089, is equally applicable to the existing HOC.

Given this hook is just a new interface to an existing library, shouldn't we start with an implementation that's as similar as possible to the HOC, warts and all? Then the next pass can be to make improvements to both the hook and HOC interfaces.

[felthy]

The below change fixes SSR for me, and it passes all existing and new tests:

diff --git a/packages/react-jss/src/createUseStyles.js b/packages/react-jss/src/createUseStyles.js
index 3b23952..d72c85a 100644
--- a/packages/react-jss/src/createUseStyles.js
+++ b/packages/react-jss/src/createUseStyles.js
@@ -55,11 +55,14 @@ const createUseStyles = <Theme: {}>(styles: Styles<Theme>, options?: HookOptions
       let dynamicRules
       let classes
       if (sheet) {
-        if (context.registry) {
-          context.registry.add(sheet)
-        }
         dynamicRules = addDynamicRules(sheet, data)
         classes = getSheetClasses(sheet, dynamicRules)
+        manageSheet({
+          index,
+          context,
+          sheet,
+          theme
+        })
       }
 
       return {
@@ -71,7 +74,7 @@ const createUseStyles = <Theme: {}>(styles: Styles<Theme>, options?: HookOptions
 
     useEffectOrLayoutEffect(
       () => {
-        if (state.sheet) {
+        if (state.sheet && !isFirstMount.current) {
           manageSheet({
             index,
             context,

[HenriBeck]

@kof would you be fine with changing the filtering of not attached sheets? I don't think to attach the sheets on the server is the correct solution and is really only an "unwanted" side effect of the hoc

[kof]

Probably, need to see what tests break once you remove that check from sheets registry. I recall it was added because there was a case sheets were added to the registry even though they were never attached, which means they don't need inlining as critical.

Basically .attached check in toString is the mechanism that prevents critical CSS from bloating.

[HenriBeck]

Probably, need to see what tests break once you remove that check from sheets registry. I recall it was added because there was a case sheets were added to the registry even though they were never attached, which means they don't need inlining as critical.

The SheetsRegistry should only be used on the server, right? In that scenario, there shouldn't be any sheet created which shouldn't be attached.

[kof]

The SheetsRegistry should only be used on the server, right? In that scenario, there shouldn't be any sheet created which shouldn't be attached.

I have tried to understand git blame changes there (I wasn't good at atomic PR's there) ... I think the problem might have been that the SheetsRegistry was used in some cases on the client too, since it's a very generic thing. It could be a more rare cases where people build some CSS editing tools (heared of that too).

[kof]

I think we can make the attached check optional with .toString({attached: null}) or something:

toString(options?: ToCssOptions = {attached: true}): string {
  return this.registry
    .filter(sheet => options.attached == null ? true : sheet.attached === options.attached)
    .map(sheet => sheet.toString(options))
    .join('\n')
}

That way it's also backwards compatible and we don't have that problem any more and don't have to attach on the server in react-jss.

[HenriBeck]

I think we can make the attached check optional with .toString({attached: null}) or something:

toString(options?: ToCssOptions = {attached: true}): string {
  return this.registry
    .filter(sheet => options.attached == null ? true : sheet.attached === options.attached)
    .map(sheet => sheet.toString(options))
    .join('\n')
}

That way it's also backwards compatible and we don't have that problem any more and don't have to attach on the server in react-jss.

But then I would enable this by default to get all of the sheets because most users will use it on the server.

[kof]

Right, if we don't enable this by default and remove attach() call, SSR extraction would always need to use that option, that would be bad.
Seems like we have to do a breaking change here, yes.

[HenriBeck]

Even though it's a breaking change, it will only affect a few users.

[kof]

@felthy check out #1148

[kof]

10.0.0-alpha.22