SSR should handle query errors

[tmeasday]

See #404

If your server throws an error for a query, then AC will throw an ApolloError, but it will be unhandled by this line: https://github.com/apollostack/react-apollo/blob/master/src/server.ts#L131

We could either

a) try and handle the error on that line (by catching the exception), and maybe just not recursing.

return query.then(_ => getDataFromTree(element, context, false))
// don't try and fetch the subtree if there's any error, maybe log it though
.catch(e => null);

OR, we could

b) handle the error here: https://github.com/apollostack/react-apollo/blob/master/src/graphql.tsx#L361:

return observable.result()
.catch(e => return { error: e });

and thus allowing the subtree to render in the error state.

I guess (b) is more correct, although maybe more complicated in that we need to keep the error "rendering" behaviour in sync between fetchData and render, which is maybe wasted effort given that it seems unlikely they are going to fetch any more queries in their error handling rendering (and that's the only reason to keep recursing on the server).

OTOH if we don't do it I think there'll be the occasional problem people run into where the different behaviour confuses them.

What do you think @jbaxleyiii?

[jbaxleyiii]

@tmeasday I lean towards option b though I think there is still some questions around error handling in general.

1. What do we do on a network error?

• if the endpoint 404s there doesn't seem to be any reason to continue
• if it is a 504 should we try to make to make other queries

2. How are individual query errors handled (i.e. some data error on the GraphQL server)

• for this case I think the UI should handle the error just like it would client side

I don't fully understand this:

although maybe more complicated in that we need to keep the error "rendering" behaviour in sync between fetchData and render

[tmeasday]

What do we do on a network error?

I would be inclined to not overthink it; it's unlikely a page has more than one or two queries (and they probably are going to all fire straightaway), so firing off extra queries on 404 is probably not a big problem. Plus it's like a misconfiguration problem rather than something that would happen in production.

So I think we treat each query separately.

for this case I think the UI should handle the error just like it would client side

I tend to agree with this.

I don't fully understand this:

although maybe more complicated in that we need to keep the error "rendering" behaviour in sync between fetchData and render

Oh, I realise now that I was thinking about it wrong. The "result" of fetchData's promise isn't really relevant, we just need to wait for it resolve before trying to render again. So we just want fetchData's promise to just not error in this case.

What will AC do if you run a query that has previously network errored (in the rendering phase of SSR)?

I suspect it may try and send the query again, which is not what we want. (Will report back)

[tmeasday]

Hmm, OK trying a few things with AC, it doesn't seem like if you:

1. Run a query, which errors.
2. Run the same query again

There is anyway for 2. to return an error without making another network request. If you pass noFetch, it'll just return no data, rather than the error.

@stubailo / @helfer I suspect what we want here (for two different ObservableQuerys, which both have the same arguments, to share errors) is kind of incompatible with current AC design. Is this right?

[techbos]

Any updates?

[bensalilijames]

For a short term solution, could we simply adopt @tmeasday's suggestion of doing:

return observable.result().catch(e => return { error: e });

And make do with the additional network request? ATM causing the whole app to crash on a resolver error (e.g. user not authorised) isn't ideal for SSR!

[tmeasday]

@benhjames I think you are right; I got hung up on trying to figure out a solution that didn't involve an redundant request but we should just fix the actual problem.

Sorry about the delay here folks, I'll take a look at this today.

[tmeasday]

Ok, I reminded myself what was going on here. The only issue is that because of the failure to share errored queries mentioned here, we will end up rendering the query in "Loading" state.

SEE #488

[calebmer]

Fixed in 1.0.0-rc.3 🎉

You still have to handle errors, but now we won’t stop resolving errors just because one rejected.