New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that SSR completes when the GraphQL server throws errors. #488

Merged
merged 9 commits into from Mar 17, 2017

Conversation

Projects
None yet
3 participants
@tmeasday
Copy link
Contributor

tmeasday commented Feb 27, 2017

See #406.

We still end up rendering a loading screen but that is better than
just bailing out of SSR completely.

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
Ensure that SSR completes when the GraphQL server throws errors.
See #406.

We still end up rendering a loading screen but that is better than
just bailing out of SSR completely.
// we will "forget" this when the "rendering" SSR runs (i.e. we will
// re-run the query, and rendering in a loading state).
// If we change that in future, it may be worth running `getDataFromTree`
// on the subtree, just in case the user runs subqueries in the error state

This comment has been minimized.

@tmeasday

tmeasday Feb 27, 2017

Author Contributor

Or is this just a silly thing to worry about?

This comment has been minimized.

@helfer

helfer Feb 28, 2017

Member

I'm not sure what the current behavior is, but I think if the query errors during SSR it's better to have it re-attempt loading on the client rather than showing an error state and not automatically retry. Does that sound reasonable?

This comment has been minimized.

@tmeasday

tmeasday Feb 28, 2017

Author Contributor

That makes sense, and I guess it's what will happen with the current behaviour, definitely[1]

The "silly thing" I'm talking about though is if there are sub-queries inside the "error" UI code path. Realistically I think it's probably fine if we don't support that.

[1] I wouldn't expect it would make much sense to pass error states over in hydrated data for instance.

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Feb 27, 2017

@calebmer (not sure who is best to ask) - do you think we should open an issue against apollo-client about re-using errored queries? I'm not sure it's a great "feature" but I can't quite see how to make the SSR render the error otherwise.

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 1, 2017

@tmeasday what do you mean by re-using errored queries?

What I would expect to happen is that SSR renders the React component with an error as it would on the client. Does that not happen?

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Mar 1, 2017

It doesn't because of the way that the SSR mechanism works:

  1. We "fake" render the app, picking up all the queries that are needed for the current url
  2. We execute the queries, and wait for them all to complete (one way or another, including errors)
  3. We then actually render the app with renderToString(), but using the same client cache.

As renderToString renders a single time and cannot "wait", this only works because in step 3 the results fetched in step 2 are re-used (and for instance we ignore forceFetch if you set sslMode: true to ensure reuse).

However, because errored queries are not reused (which is sensible for other reasons), this means that during the renderToString run, a new query is created and is of course in the "loading" state.

Does that make sense?

I guess we should make it so that in ssrMode we do re-use errored queries?

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 2, 2017

Yeah, I’m not sure how comfortable I am with step 1. That should probably be something we revist in the future.

What do you mean by re-using errored queries? Do you mean not throwing an ApolloError when we have GraphQL errors (which is something we should probably do anyway)? Do you mean saving errors in Redux state in step 1 and reconstructing those errors in step 3?

Ideally I’d like to see a way for users to catch any errors and then choose how to handle that. They could either render nothing, opting for a full client render. Render a loading component, or render an error screen. Would that work, and/or how close is this PR to allowing users to handle this kind of error?

Right now there isn’t really a nuanced way to handle errors in apollo-client.

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Mar 3, 2017

Yeah, I’m not sure how comfortable I am with step 1. That should probably be something we revist in the future.

Is there an alternate way to do it? Definitely it's a bit janky but I don't think aside users manually specifying queries at the route level there is a better approach.

Do you mean saving errors in Redux state in step 1 and reconstructing those errors in step 3?

Yes, this is what I mean. Currently if you run a second version of the same query against apollo-client without forceFetch it will make a network request if the first version threw an error. I'm not against this behaviour, but hopefully it's clear why it's a issue for the SSR approach above.

Ideally I’d like to see a way for users to catch any errors

We could definitely make the getDataFromTree() promise throw if there's an error for any of the queries found in the app. Is that what you mean?

I'm not sure if that should be the default, but it would be easy to implement. Perhaps as a separate PR?

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 3, 2017

@tmeasday

Is there an alternate way to do it?

The best thing to do is probably implement a custom asynchronous React renderer. This would also have the benefit of us being able to stream HTML as queries resolved which would be sweet 😊. Hopefully React Fiber will make this easier, and perhaps also force us to reconsider as this approach may just break.

Although personally I would prefer users to manually specify queries at the route level. We won’t go that direction at this point, however.


In order for users to catch errors and handle it however they want in their own apps we wouldn’t have to change anything, correct? Given that a query promise will be rejected if it failed and then that rejection will propagate to the Promise.all. Why is this not sufficient for SSR users to render a loading or error component?

We know that our current SSR approach does have some limitations, this is one of them. So do we keep patching over those limitations adding complexity in the process, do we choose a fundamentally better approach, or do we accept the tradeoffs and use reasonable workarounds? On principle I generally prefer the latter two.

I currently perceive the cost of the patch to apollo-client as relatively high and the cost of the missing functionality as relatively low, but if the cost of the patch is lower then the cost of not having this functionality then we should pursue getting a patch into apollo-client. So are my perceptions wrong? How costly (in increased code complexity, future bugs, etc.) would a patch be, and/or how bad is not having this functionality especially if we provide a reasonable workaround to the server crash problem?

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Mar 4, 2017

The best thing to do is probably implement a custom asynchronous React renderer.

Maybe we could repurpose the one from next.js[1]? I agree this is the ideal approach; if a fair whack more involved than what we are doing, though.

[1] At first blush our fetchData =~ getInitialProps

In order for users to catch errors and handle it however they want in their own apps we wouldn’t have to change anything, correct?

I suppose you are right; I guess this makes sense in a context where server errors are critical; is there a legitimate use case where you would want your SSR to continue to work even though the server has thrown an error?

I guess I am asking do we want SSR to work if there are non-critical errors thrown by the GraphQL server? I'm OK with saying it's a limitation of our current approach if we think that's rare.

If we go down this road we should add a exception handler to the example: http://dev.apollodata.com/react/server-side-rendering.html#getDataFromTree

Having said that, this change (to instead not throw errors up and instead just render what we can) is pretty minimal; although it does preclude handling errors "critically".

I currently perceive the cost of the patch to apollo-client as relatively high

I'm confused by exactly what you are referring to by "the patch"? Are you saying a patch to AC to "reuse" errored queries? I definitely agree that the added complexity is in no way worth it; especially when you put it in terms of this current SSR approach having a limited shelf life.

[If you are referring to this PR, then I'm not sure I agree but I suspect you aren't?]


If that is correct then it's a question of which limitation we'd prefer to accept; treating all SSR errors as critical w/ no way to continue; or treating none as critical and confusingly rendering loading states on the server.

I'm honestly unsure which is better.

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 6, 2017

@tmeasday

Are you saying a patch to AC to "reuse" errored queries?

Yes, that is what I meant. Sorry for lacking clarity 😊

Maybe we could repurpose the one from next.js?

I didn’t know Next.js was using a custom renderer! Do you have a link? 😊

If they are then it would be much easier for us to reach the ideal SSR implementation then I thought. If you want to open an issue for that this may be worth discussing.

If that is correct then it's a question of which limitation we'd prefer to accept; treating all SSR errors as critical w/ no way to continue; or treating none as critical and confusingly rendering loading states on the server.

If we keep things as they are there is nothing stopping users from ignoring the error and rendering loading states. They would just call getDataFromTree(...).catch(() => { /* noop */ }). To make this possible we should replace Promise.all with a custom implementation that only resolves if all the promises have either resolved or rejected. Promise.all currently ends early if one of the promises rejected. We may also want to collect all of the rejects and throw an array of errors (or an error with the property queryErrors), so the user has access to all the errors and not just the first. What do you think about modifying this PR to implement that behavior which would allow the user to choose between both ends of the spectrum and/or switch depending on the criticality of the error?

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Mar 7, 2017

Hmm, OK Next's SSR isn't that interesting: https://github.com/zeit/next.js/blob/master/server/render.js#L31 / https://github.com/zeit/next.js/blob/master/lib/utils.js#L45 -- they don't do any recursion at all, either within components or w/ multiple levels of promises.

To make this possible we should replace Promise.all....

I think I can change this PR pretty simply to complete every query, then throw an error afterwards, as you've suggested. I'll take a look tomorrow, if you agree. Basically the code would just be

const errors = [];
return Promise.all(queries.map(...).catch(e => errors.push(e)))
  .then(results => {
    if (errors.length) {
      throw errors;
    } else {
      return results;
    }
  });
@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 7, 2017

Yeah, that’s how I thought Next.js work. They only load asynchronous props from the root component and then use the react-dom/server renderer.

Yep, that’s what I was looking for. Although for the errors maybe we should do something like:

if (errors.length > 0) {
  const error = errors.length === 1
    ? errors[0]
    : new Error(`${errors.length} errors were thrown when executing your GraphQL queries.`);
  error.queryErrors = errors;
  throw error;
}

Which should provide the most information and the best debugging experience 😊

@tmeasday tmeasday self-assigned this Mar 8, 2017

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Mar 14, 2017

@calebmer apologies this dropped off my radar due to technical and life reasons :).

Should be good now.

calebmer added some commits Mar 14, 2017

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 14, 2017

Looks good to me 😊

Made some small changes, so I’ll merge once CI passes 👍

@calebmer calebmer merged commit fb70659 into master Mar 17, 2017

5 checks passed

./dist/index.min.js No change (49,014 bytes)
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.8%) to 92.708%
Details

@calebmer calebmer deleted the 406-ssr-errors branch Mar 17, 2017

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 17, 2017

Released in 1.0.0-rc.3 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment