Refactor to calculate data at render time #265
Conversation
| @@ -191,14 +191,18 @@ export default function graphql( | |||
| return opts; | |||
| } | |||
|
|
|||
| function shouldSkip(props) { | |||
| return mapPropsToSkip(props) || (mapPropsToOptions(props) as QueryOptions).skip; | |||
There was a problem hiding this comment.
this does end up calling mapPropsToOptions more often than before. Dunno if that's a problem.
There was a problem hiding this comment.
Yeah, we should probably just do that once in willRecieveProps (and initially).
I wouldn't expect people would have assumptions about how many times it's called though.
|
@tmeasday any updates on this? |
|
Good question, I'll try to get back to this next week. Would be a useful change I think. |
tmeasday
force-pushed
the
264-data-at-render-time
branch
from
c9ee4b4 to
cc7eadd
Compare
tmeasday
changed the title
|
@jbaxleyiii this is now ready to go. |
|
Ok, there's a bug in AC that needs to be addressed, so hold fire until then (it's causing the test to timeout) |
tmeasday
force-pushed
the
264-data-at-render-time
branch
from
5ee10ae to
bd2f887
Compare
|
@jbaxleyiii I'm not sure if you want to have a final review of this, but this is ready to go. Would like to merge it into the next release. |
|
@tmeasday on it! |
|
@tmeasday man thats fantastic! Go for it! |
| this.data = assign({}, defaultQueryData) as any; | ||
| this.createQuery(opts); | ||
| } | ||
| this.createQuery(opts); |
There was a problem hiding this comment.
It looks like we now call createQuery unconditionally on setInitialProps (for non-mutations), even if the query is initially skipped. Isn't that wrong?
There was a problem hiding this comment.
I think we are OK now because we don't even get to setInitialProps if we are skipping: https://github.com/apollostack/react-apollo/blob/bd2f887a559c2fe1ba7b3b89e9b337e3223a9402/src/graphql.tsx#L261
There was a problem hiding this comment.
Ah, whoops, I was imagining that setInitialProps was a React lifecycle API like getInitialState.
WIP for #264
Issues outstanding
observableQuery.currentResult()should returnerrorsalso. Need to PR AC for that.previousData. Moving forward we should just be able to blindly passcurrentResultthrough; perhaps AC should cachepreviousData. But hopefully what I'm doing works for now.