Refactor ssr

[tmeasday]

Refactored SSR to use a slightly more principled walkTree method.

Tests pass, some points of interest:

1. I got rid of returning the context from the SSR functions. This seemed kind of undefined (what was it? The lowest context we saw in the tree? Should we have been just picking the first ApolloProvider child context we saw?). All in all it seemed a bit hacky, and the user will have access to an ApolloClient anyway, so can't they just get the store themselves?

• this will require a docs change.

2. I am still assigning instance.setState. This seems to not throw an exception. How does this fit your understanding @rewop? I was surprised by this but we need to do something like this to allow people to call setState in componentWillMount (or pursue a different method of rendering[1]).

3. @jbaxleyiii I wonder if rather than passing { element, context } around (and calling element.type.fetchData, a static method), we could not just pass the instance around, and call instance.fetchData? It would make the code a little simpler in server.js, but also it would make graphql.js a bit simpler too. Is there a reason to not instanciate the element on the server before running the query?

[1] I feel like ultimately we should be trying to use React's rendering infrastructure, as although this is no doubt quicker, it feels really brittle and wouldn't be surprised if there's lots of things you can do that break it.

[tmeasday]

I believe this will fix #237 and #250

[rattrayalex]

Is there a reason to not instantiate the element on the server before running the query?

I'm just a bystander with little context, but I imagine performance would be one reason – especially if users defined any heavy constructor methods, but even with standard object/class instantiation, that could add up with an entire react component tree.

[tmeasday]

I'm just a bystander with little context, but I imagine performance would be one reason – especially if users defined any heavy constructor methods, but even with standard object/class instantiation, that could add up with an entire react component tree.

Sure, but we have to create the instances at some point to render the subtree. Also in this case we are talking about graphql instances anyway, and we know a lot about their behaviour!

[rattrayalex]

Asking out of curiosity, why is it necessary to call setState? I don't really see any relevant usages of setState within the react-apollo codebase

Feel free not to answer if it's complicated 😄

[tmeasday]

We don't call set state, but it's totally possible that any component we are "rendering" will rely on calling in it componentWillMount. I think there were some issues about this (and that's why there's a test for it).

[rattrayalex]

Ahhh, I see. That makes sense, I believe.

I imagine another option might be to run Component.flushState() or whatever it's called, just before Component.componentWillMount() ? But I don't necessarily see that as more elegant.

Again, thanks for taking the time to explain!

[rattrayalex]

Again, asking out of curiosity, no need to respond (I just find this to be a really interesting project):

Won't this prevent queries on child components from executing?

[tmeasday]

True, but it will eventually get rendered when the query is ready. The assumption is that you have something like

@graphql(...)
const X = ({ loading }) => {
  if (loading) {
    // A. there's no nested calls to `graphql` inside this render tree
    return <Loading/>;
  } else {
    // B. there could be anything in here
  }
}

We don't bother rendering the A. subtree as we are going to await the query setup by graphql and then render X with the results (and loading: false). There are probably pathological cases you can think of where this is inefficient, but I think it seems a reasonable assumption and is "eventually consistent" in any case.

[rattrayalex]

Hmm, is Component.fetchData recursive then? What if the B subtree has a @graphql(...) of its own?

Thanks very much for taking the time to explain!

[tmeasday]

getQueriesFromTree is recursive, so B's @graphqls will get picked up after X's query is loaded.

We have to wait for X's query though, as any sub-queries may rely on the results.

Thanks very much for taking the time to explain!

No problem! Happy to talk about it while I still remember ;)

[rattrayalex]

Ahhh, of course. Somehow I missed the fact that there are two separate recursions happening: one to find all top-level graphql nodes, and one to find/execute their child queries once the first have completed... although, of course, you had clearly stated that.

We have to wait for X's query though, as any sub-queries may rely on the results.

Of course. Presumably in the future some introspection could check whether this is true, and batch the queries together if not.

Hah, well I'm glad to get your thoughts on the public record while we can then! 😄

[rattrayalex]

Wow, what a massive cleanup!

Are you planning to keep initialState as a return value?

[tmeasday]

No, see point 1. in the original PR comment; it was a bit of a hack IMO

[rattrayalex]

Ahh, I see...

the user will have access to an ApolloClient anyway, so can't they just get the store themselves

Yes, that sounds much better anyway 😄

[rattrayalex]

This PR seems like a major step forward in the simplicity and readability of this functionality!

[jbaxleyiii]

@tmeasday awesome work! I'm going to try and review this today!

[rewop]

@tmeasday sorry I was away on holiday. This PR is great indeed. As for my issue the problems was setting props and context to the instance of the component because the properties could not be modified. setState was not an issue for me.

So this should fix my scenario.

[rewop]

I feel like ultimately we should be trying to use React's rendering infrastructure, as although this is no doubt quicker, it feels really brittle and wouldn't be surprised if there's lots of things you can do that break it.

Totally agree with this.

[rattrayalex]

I feel like ultimately we should be trying to use React's rendering infrastructure, as although this is no doubt quicker, it feels really brittle and wouldn't be surprised if there's lots of things you can do that break it.

Speaking as a (potential) user of this library, perf is likely to be a top consideration in whether to adopt Apollo for SSR. I don't know very much about the tradeoffs at hand, so I wouldn't take my "opinion" on the matter too seriously. But If tests can be written to combat the brittleness and detect breakages, that'd be much preferred to a slower-but-more-"correct" approach.

Writing some benchmarks might be helpful in making that call as well (though again, I don't have the context necessary to tell how helpful that'd be).

[jbaxleyiii]

@tmeasday this looks awesome. I'm great to merge it if you are! (Feel free to merge and release if you want!)

[tmeasday]

I think it's a step forward in any case, so I am going to merge it. I guess we'll hear about problems as it goes live ;)

[tmeasday]

@jbaxleyiii I wonder what your thoughts are on

I got rid of returning the context from the SSR functions. This seemed kind of undefined (what was it? The lowest context we saw in the tree? Should we have been just picking the first ApolloProvider child context we saw?). All in all it seemed a bit hacky, and the user will have access to an ApolloClient anyway, so can't they just get the store themselves?

[jbaxleyiii]

@tmeasday it was a hack 👍 I think its great

[jbaxleyiii]

Also, thank you SO much for the fantastic work with the new SSR renderer!

[tmeasday]

Ok, I'll update the docs etc to reflect the change.