cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.06k stars 397 forks source link

SSR mismatching classes with client #676

Closed wouter-willems closed 6 years ago

wouter-willems commented 6 years ago

Hi, I use SSR to render pages and rehydrate them client side. JSS is used to create our stylesheets. Now I just updated to the latest version (9.6.0) and noticed there are now class mismatches between server and client, breaking our app. I did some digging and found out that the bug was introduced between 9.3.3 and 9.4.0. This means the breaking change is somewhere between these commits: https://github.com/cssinjs/jss/compare/8298094e47d8701877202c6752cdc5b3ff01a43e...0e2b290031badb612019cc2c9708ba42976eb928

Maybe the jss.js file concerning the instance counter?

I would be happy to help with a codepen.io or something, but not sure how to best set that up since SSR is also needed to show the problem.

wouter-willems commented 6 years ago

It seems indeed that if I remove the changes in jss.js, namely the instanceCounter = 0 and id=instanceCounter++, everything works like a charm again.

But im sure it was added for a purpose ;)

kof commented 6 years ago

It seems that you create a different amount of Jss instances on the server and on the client. Maybe you are doing some sort of hot reload on the server? Is it production? Is it versions mismatch between server and client?

In any case I need a reproducible code. You can probably use codesandbox and simulate ssr by rendering to string and rehydrating from there.

kof commented 6 years ago

Created a feature request that will help to debug such mismatches in the future https://github.com/cssinjs/jss/issues/677

kof commented 6 years ago

As I have referenced this issue in the other issue, we can close it, most probably resolution will be in your code.

rogierslag commented 6 years ago

This is not really a strange case. Imagine certain code lazy loading on the client (which would not get rendered on the server): that would also cause a different number of classes to render on both.

All in all the global state seems like a hack in the architecture.

Also note that if there is some kind of mismatch somewhere, JSS simply unmounts everything, usually without any proper error message.

wouter-willems commented 6 years ago

This is still an issue. We can not upgrade to newer versions of jss because of the instance counter issues. Are there any updates on this?

kof commented 6 years ago

As you said the problem with mismatch seems the instance counter, you create more instances of jss on the server than on the client it seems. If you post here 2 mismatching classes, one generated on the server one on the client, we can see exactly what part failes

class name id consists of {moduleid}-{instanceid}-{counter}

alexwalterbos commented 6 years ago

I've picked up this issue in our code base from @wouter-willems.

It seems that you create a different amount of Jss instances on the server and on the client.

@kof You were right about this, we were indeed creating multiple Jss instances during SSR. Our apologies for whatever extra effort this cost you! To help others encountering this issue:

The name mismatches between the classes from the server and the client showed us that the multiple jss instances were the issue. The structure of the classname is {moduleid}-{instanceid}-{counter}, and the instanceid differed between our server-provided classes and our client-generated classes. Be careful of when you create your jss instances. We called createJssInstance() in the props of our root JSX tag:

const instance = <App
   { ...otherProperties }
   jss = {createJssInstance()}
/>

Instance is actually a function, since the JSX here is syntactic sugar for React.createElement(App, <props>). Whenever this was (re-)rendered during SSR, it called createJssInstance() while collecting the props and that incremented the counter yet again. This is the fix:

const jssInstance = createJssInstance(); // Instance is now created on the scope of the file, or at least the parent scope
const appInstance = <App
  { ...otherProperties }
  jss = { jssInstance }
/>

@kof thank you for maintaining this library, it has been a great asset in our stack. We would still very much appreciate the proposed rehydration function from #677.

kof commented 6 years ago

@alexwalterbos thank you very much for helping clarify this. Yes I totally want to fix #677 among of many other very important things and I just can't afford enough time. I would appreciate any help. I am always open to discuss issues here or on gitter/twitter so that you have easier time to understand how things work and why.

alexwalterbos commented 6 years ago

No problem at all, just paying it forward! I'd love to contribute to jss, I'll see if there's anything I can do in the near future. I'm sorry I can't be more clear about that, but my schedule is not very permitting at this time; I'm sure you know the feeling. I will be in touch!