Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rehydration #379

Closed
PepijnSenders opened this issue Dec 7, 2016 · 23 comments
Closed

Rehydration #379

PepijnSenders opened this issue Dec 7, 2016 · 23 comments
Labels
question Documentation is not good enough.

Comments

@PepijnSenders
Copy link

PepijnSenders commented Dec 7, 2016

Hey guys, was wondering why you guys don't implement client rehydration like Aphrodite and Glamor do?

Quoting from your documentation:

Once js on the client is loaded, components initialized and your jss styles are regenerated, it's a good time to remove server-side generated style tag in order to avoid side-effects, example in react.

Isn't this a bit wasteful, since all the CSS is already generated?

@kof
Copy link
Member

kof commented Dec 7, 2016

You mean we should not generate CSS from JSS if website has been loaded with CSS already?

@PepijnSenders
Copy link
Author

Yes, since it has already been generated by JSS on the back-end.

@kof
Copy link
Member

kof commented Dec 7, 2016

That would mean we need to parse the generated CSS and bootstrap all the models into JSS, basically same like react does it. Otherwise models are out of sync.

@kof
Copy link
Member

kof commented Dec 7, 2016

I am not sure what other libs do in that case, but they may do this because they don't have models, they simply generate css?

@kof
Copy link
Member

kof commented Dec 7, 2016

Also I am not aware of any perf. bottlenecks in case of regeneration of CSS the way we do it now, it is actually very cheap. Not to be compared with reacts DOM

@PepijnSenders
Copy link
Author

Yeah me neither, not sure how CSS takes care of that in the browser, but I can imagine if you have a lot of styles doing the same regeneration again for all the components is a bit wasteful. Don't you think?

@kof
Copy link
Member

kof commented Dec 7, 2016

From what I know, you are talking about 5ms in the most cases. CSSOM is not a perf bottleneck like the DOM.

@PepijnSenders
Copy link
Author

I think aphrodite and glamor both keep an array of the edited ID's when creating the classNames. They also both require the ReactDOM to be rendered at the same time as the CSS. So this way they are sure nothing will be rendered outside of that scope.

@PepijnSenders
Copy link
Author

Yeah I'm not that worried about CSSOM, the JS bottleneck was what kinda worried me. But yeah probably we're talking about miliseconds here.

@PepijnSenders
Copy link
Author

Probably a good idea to investigate and maybe make a note on it in the documentation?

@kof
Copy link
Member

kof commented Dec 7, 2016

I think we should invest time into it once someone has a prob. We have a lot of critical issues.

@kof
Copy link
Member

kof commented Dec 7, 2016

If you want to take a deep look into it I am always happy to help and improve if needed.

@kof kof added the question Documentation is not good enough. label Dec 7, 2016
@kof kof closed this as completed Dec 7, 2016
@kof
Copy link
Member

kof commented Dec 7, 2016

Also if you are looking to contribute, take a look at the issues with important label.

@PepijnSenders
Copy link
Author

👍

@jescalan
Copy link

Hi guys! So I was just thinking about this recently. It rubs me the wrong way that with server rendering, you basically render once on the server, generate a full set of css and classes, then on the client you regenerate the css and a new set of classes and remove the server-rendered css.

I haven't measured the performance impact, but I can't imagine this is the most efficient way, and additionally, it requires a but more work to implement, and there's some cognitive dissonance to "rehydration", since nothing is actually being rehydrated, and rather we are just starting from scratch on the client side and wiping out the server-generated styles.

It seems feasible that this could be done, especially with comment hints injected into generated css. I'm going to think about this a bit and maybe submit a PR with some ideas soon 😁

@kof
Copy link
Member

kof commented May 25, 2017

Ideas are always welcome. But start measuring first!

@jescalan
Copy link

jescalan commented May 25, 2017

Ah also, something that might help a lot would be to generate a separate style tag for each component and drop a unique id on them so it's super quick and easy to parse what's needed. This seems to be the approach taken by styled-components. This however would be a breaking change, since the style tags would be generated, where right now just the css string is generated from toString()

I think this issue should be re-opened. I know it's not on the top of your list of important things, but it definitely is an open and active area for improvement, so no reason to have it closed!

@kof
Copy link
Member

kof commented May 25, 2017

Please proof first that what we do now is not efficient.

@jescalan
Copy link

Ok so while I obviously don't have proof yet, because proof would require full implementation of the rehydration technique for comparison, I feel like it's pretty likely that this change will be a perf win, and here's why. Any thoughts definitely appreciated, since it would be a waste to go down this path and end up being wrong!

First, with the exiting method, every app using JSS is shipping two full copies of the entire app's CSS to the client. One is the css version that is server rendered on to the page. The second is the jss that is required individually into every component so that the library can regenerate the css styles on the client side. While both are gzipped, they are formatted differently and in different files, so gzip will not actually reduce any of the duplication between them. For apps with a lot of CSS, and which require good performance on weak networks (this represents a LOT of apps), I feel like this point alone will easily make up for the performance difference, because we're talking about doubling the full size of the css and delivering both over the network.

Second, with the existing method we have the following workflow:

  • jss parsed into css
  • node created, css added to node, node added to page
  • previous server rendered node removed from page

With the proposed flow, it would change to:

  • css parsed into jss

The only situation in which I can see the existing method performing better is if parsing jss -> css is so significantly faster than the reverse direction that it negates both the extra dom mutations, and shipping the full css twice, which seems pretty unlikely to me, although I would guess it would become more likely the more plugins you use. You would certainly know more here than me though, so any thoughts appreciated!

@kof
Copy link
Member

kof commented May 25, 2017

First, with the exiting method, every app using JSS is shipping two full copies of the entire app's CSS to the client.

How is the inlined critical CSS the entire app?

@jescalan
Copy link

Fair enough. Will tinker with this and get back to you 😁

@kof
Copy link
Member

kof commented May 25, 2017

Also the problem is how do you avoid loading the part of css, that was already inlined within critical css, within your js code?

@kof
Copy link
Member

kof commented May 25, 2017

Also there are different kinds of critical css. If you load a dummy layout as a critical css, you can completely avoid any duplication or redundancy. But you will not have any meaningful information within initial load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Documentation is not good enough.
Projects
None yet
Development

No branches or pull requests

3 participants