Open abecks opened 6 years ago
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:
Meteor.EnvironmentVariable
to access SSRDataStore
in place of React's Context API. Provides access to the store and userId from the fetch
methods. onPageLoad
and serve it with the page similar to SSRDataStore.getScriptTags()
heregrapher
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
:
httpOnly
cookie with their login tokenonPageLoad
or WebApp
callback, read the cookie@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:
DataHydrator.destroy()
must be called after initial render./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
new React.Context
will break backwards compatibility. I'm unsure what to do about that, any suggestions?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.
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 allwithQuery
components.withStaticQuery
andwithReactiveQuery
contain logic to immediately fetch a query result, store it, and retrieve it again on the client.FastRender
uses, however it may require modification of grapher core (I haven't looked yet). Static Queries are not loaded twice.grapher
core in any way.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.