Add automatic SSR data hydration support

[abecks]

Target issue: cult-of-coders/grapher#199

Adds data hydration support for server-side rendering.

Hey @theodorDiaconu
I'd love to get your opinion on this approach:

Uses a similar approach to styled-components. Creates a new data store for every request, and uses the new React Context API to expose it to all withQuery components. withStaticQuery and withReactiveQuery contain logic to immediately fetch a query result, store it, and retrieve it again on the client.

1. Subscription data is fetched twice (once on server, once on client). I can improve this in the future by marking the subscription as ready with the same techniques FastRender uses, however it may require modification of grapher core (I haven't looked yet). Static Queries are not loaded twice.
2. The client-side hydration data store self destructs after 3 seconds (configurable). The hydration store should be considered old and invalid after the initial request is served. Any dynamically loaded components should be fetching from the server instead of from the store.
3. This does not require modifying grapher core in any way.
4. Using a per-request store instead of a global store makes it easier and more secure to accumulate data. No userId checking required.

There are no tests for this repository, so I wasn't able to run any, but I did test static, reactive, and singular queries pretty extensively.

[theodorDiaconu]

propper 4 space indentation please

[theodorDiaconu]

specify defaults outside the function itself

[theodorDiaconu]

Also, why 3s for self-destructing ? Isn't that unsafe ?

[theodorDiaconu]

This release is going to break React 15.x, we need to make it optional. We still want to allow latest versions of grapher-react to be used with 15.x.

[theodorDiaconu]

use consistent Coding standards (space before {)

[theodorDiaconu]

I now understand why you are doing the self-destruct thingie. But a healthier approach would be to set a flag once first hydration has been made, and do the check in here. I don't know something like window.grapher.hydrationCompleted = true

And pls use consistent styles if[SPACE](...)

[theodorDiaconu]

create 2 additional methods for this, there's too much code coupled in the constructor.

[theodorDiaconu]

what if the query gets an error ? how is this treated ?

[theodorDiaconu]

I fail to understand why you need context, if you can use window as a global variable to do your thing.

window.GRAPHER = { queryStore: {}, hydrationCompleted: bool, dataStore }

[theodorDiaconu]

Hello @abecks good job and great initiative, I really enjoyed reviewing the code. However I have the following concerns:

How do we address security ? The biggest flaw we now expose is for named queries, that may get filtered based on the current user. In order for this to be solved, we need to have a solution (not in this package) that automatically translates auth token as cookie, on server we read and we translate it to an userId, and then it can get passed to the .fetch(context). Example:

namedQuery.fetch({ userId: 'XXX'})
normalQuery.fetch({ userId: 'XXX'})

This ensures that the firewalls are called and emulates a sort of client request.

My next concern is the self-destruction after 3s. I think I left a comment where we can set a flag to verify if the data has passed the initial rehydration. And we can simply destroy the store then.

[theodorDiaconu]

use text/grapher-data, as a constant variable somewhere. Let's not rely on strings for things that are re-used in multiple places.

[abecks]

Hey @theodorDiaconu ,

Thanks for taking the time to do such a thorough review. Regarding the code consistency, do you have an editorconfig, eslint config or prettier config that we can put in the repo? I tried to manually follow the formatting you were using but as you can see, manually formatting code is sloppy.

Using React's Context API: I'm using the context on the server, it isn't necessary on the client. On the server it acts as a per-request store. It's possible we could move all of this functionality of grapher-react and into grapher core if:

• We use Meteor.EnvironmentVariable to access SSRDataStore in place of React's Context API. Provides access to the store and userId from the fetch methods.
• We can then grab that data in onPageLoad and serve it with the page similar to SSRDataStore.getScriptTags() here
• Using React's Context API was a quicker way for me to prove this concept, but I think moving this functionality to grapher core would be better.

Self destructing the store: We can manually destroy the store when hydration is finished, but it is hard to know when hydration is actually finished. If all of the React components are loaded on initial render, we can do something like this:

await DataHydrator.load()
ReactDOM.hydrate(<App />, document.getElementById('root'))
DataHydrator.destroy()

The problem here is that any dynamically imported components will load after the store has been destroyed. I guess at this point the user has an option to delay the destroy call, but it still doesn't feel right. react-loadable and it's preloadAll method may be a workaround for that. Any thoughts?

Security: Almost forgot! Thanks for reminding me. I can implement a similar method to what is used in FastRender:

• When the client authenticates, set an httpOnly cookie with their login token
• On the server, in onPageLoad or WebApp callback, read the cookie
• Fetch user object from database based on token
• Add user object to context
• Fetch methods check context for userId

[theodorDiaconu]

@abecks let's add prettier with 2 space, single-quote, and trailling-comma=es5 and it's fine by me.

Regarding security, we need to offer a recipe in the documentation for it to work, it won't be part of grapher-react package. Regarding the context, there might be a better way I have to think about it.

[abecks]

Hey @theodorDiaconu ,

I've made the following improvements:

• Added prettier and eslint config files (please double check the config)
• Reformatted all files with prettier
• Added error handling for staticQuery and reactiveQuery on the server (I would really appreciate if you could double check this)
• The DataHydrator store on the client no longer self destructs, but DataHydrator.destroy() must be called after initial render
• I have added ./lib/User.jsx with User and withUser components. Let me know if you'd rather I move this into it's own package, maybe meteor-react-auth to mirror meteor-angular-auth
• I have successfully tested firewalls with static and reactive queries
• You mentioned using new React.Context will break backwards compatibility. I'm unsure what to do about that, any suggestions?

Usage:

On the client:

import { DataHydrator, User } from 'meteor/cultofcoders:grapher-react'
import { Tracker } from 'meteor/tracker'

Meteor.startup(async () => {
  await DataHydrator.load()

  Tracker.autorun(c => {
    if (!Meteor.loggingIn()) {
      c.stop()
      ReactDOM.hydrate(
        <User>
	  <App />
	</User>,
	document.getElementById('root')
      )
      DataHydrator.destroy()
    }
  })
})

In the future I can expand the DataHydrator to inject the User object as well, so we won't have to wait for loggingIn. FastRender did this.

On the server:

import { SSRDataStore, User } from 'meteor/cultofcoders:grapher-react'

onPageLoad(async sink => {
  const store = new SSRDataStore()

  sink.renderIntoElementById(
    'root',
    renderToString(
      store.collectData(
        <User token={sink.request.cookies.meteor_login_token}>
	  <App />
	</User>
      )
    )
  )

  const storeTags = store.getScriptTags()
  sink.appendToBody(storeTags)
})

[abecks]

Hey @theodorDiaconu, any further feedback on this?

[theodorDiaconu]

@abecks very nice job!

[theodorDiaconu]

I think I need more time to analyze it I feel that bringing in meteor auth config in this package is too much. What if we split it and have something like: grapher-react-ssr (and include optional authentication modules in there) ?

[abecks]

Thanks for the feedback.

It definitely makes sense to spin off the auth functionality into something like meteor-react-auth. I can take care of that.

grapher-react-ssr could work well if we modified grapher-react to be extendable. grapher-react-ssr would need to modify withTracker in withReactiveQuery and GrapherStaticQueryContainer in withStaticQueryContainer. Then grapher-react-ssr can export it's own withQuery wrapper that uses withUser from meteor-react-auth.

I can work on making these modifications if you think we are on the right track.

[theodorDiaconu]

@abecks yes I believe that is the right path. And I will feature your packages in documentation for SSR but it's advisable that you have your own README.md on each package.

Cheers!

[theodorDiaconu]

@abecks sorry for my unresponsiveness here, any updates? I am interested in having a solution for this, and it would be a shame not to use the amazing work you did so far!

[theodorDiaconu]

ping @abecks

[abecks]

Sorry for the lack of response. Will be busy for the next few weeks. I've hit a pretty big snag, in that collection transforms are not currently run for hydrated data on the client. I want to see if I can come up with a solution for this before I release anything.