grapher-react
grapher-react copied to clipboard
Add automatic SSR data hydration support
Target issue: https://github.com/cult-of-coders/grapher/issues/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.
- 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. - 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.
- This does not require modifying
grapher
core in any way. - 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.
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.
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 accessSSRDataStore
in place of React's Context API. Provides access to the store and userId from thefetch
methods. - We can then grab that data in
onPageLoad
and serve it with the page similar toSSRDataStore.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
orWebApp
callback, read the cookie - Fetch user object from database based on token
- Add user object to context
- Fetch methods check context for userId
@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.
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
withUser
andwithUser
components. Let me know if you'd rather I move this into it's own package, maybemeteor-react-auth
to mirrormeteor-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)
})
Hey @theodorDiaconu, any further feedback on this?
@abecks very nice job!
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) ?
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.
@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!
@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!
ping @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.