Errors during SSR are rendered as "loading"

kachkaev commented 6 years ago

I'm writing a Next.js app that's based on their with-apollo example and would like to share a confusion with the server-side error handling. I believe it might be either a bug in apollo client or something we can consider as a feature request.

In the example, the logic of a HOC that deals with Apollo data is located in with-apollo-client.js and mine is not very different. When the code runs on the server, getDataFromTree() is called and then the React tree renders using the data from apollo.cache.extract(). This works fine if all requests have succeeded – the server sends a fully rendered tree with all the data, that's cool.

When a request fails during getDataFromTree(), the errors are suppressed with catch. This is generally fine, because we do not want a small faulty widget in a side panel to crash the whole page. However, because apollo.cache.extract() does not contain any information about the failed queries, failed query components render as if the data is still being loaded.

This peculiarity is a bit hard to spot in Next.js, because client-side rendering follows the server-side one and loading... gets replaced with a real error quickly (no data in cache → new query → render loading → register error → render error). However, this kind of "fix" makes the situation even harder to realise rather than resolved.

Imagine I have a page with a blog post, which is identified by a slug in the current URL. Unlike for a small side-widget I mentioned above, the success of a query that gets me the post is critical to the page. In this case, a failure means it's either 404 or 500:

import { Query } from "react-apollo";

export default ({ slug }) => (
  <Query query={BLOG_POST_QUERY} variables={{ slug }}>
    {({ data, loading, error }) => {
      if (loading) {
        return <span>loading...</span>;
      }

      if (error) {
        // fail the whole page (500)
        throw error;
      }

      const blogPost = data && data.blogPost;
      if (!blogPost) {
        // fail the whole page (404 - blog post not found)
        const e = new Error("Blog post not found");
        e.code = "ENOENT";
        throw e;
      }
      return <BlogPostRepresentation blogPost={blogPost} />
    }}
  </Query>
);

The errors that are thrown here get handled by Next's _error.js, which can render 404 - Post not found or 500 - App error, please reload using some custom logic. In neither case I want the server to return 200 even though the error will pop out on the client side shortly – search engines won't like this. The code above renders a proper 404 page when data.blogPost is null, but if a graphql server goes down for some time, all my blog posts - existing or non-existing - will return 200 and this can quickly ruin google search results for my website.

How to reproduce the issue:

Install with-apollo example

 npx create-next-app --example with-apollo with-apollo-app

Add this line to components/PostList.js:

 function PostList ({
   data: { loading, error, allPosts, _allPostsMeta },
   loadMorePosts
 }) {
+  console.log('RENDERING POST LIST', { loading, error: !!error, allPosts: !!allPosts });
   if (error) return <ErrorMessage message='Error loading posts.' />

Launch it using yarn dev and open localhost:3000 in a browser. You will see:

# server-side
RENDERING POST LIST { loading: false, error: false, allPosts: true }
RENDERING POST LIST { loading: false, error: false, allPosts: true }

# client-side (uses cache, no loading)
RENDERING POST LIST { loading: false, error: false, allPosts: true }
RENDERING POST LIST { loading: false, error: false, allPosts: true }

This looks fine.

Simulate a graphql server failure (e.g. open lib/init-apollo.js and replace api.graph.cool with unavailable-host).

Reload the page. You will still see Error loading posts., however your logs will say

# server-side
RENDERING POST LIST { loading: true, error: false, allPosts: false }

# client-side (no cache found, loading from scratch and failing)
RENDERING POST LIST {loading: true, error: false, allPosts: false}
RENDERING POST LIST {loading: false, error: true, allPosts: false}

(instead of loading: false, error: true on the server)

Turn off javascript in the browser via dev tools and reload the page.
Expected visible content: Error loading posts. Actual visible content: Loading

Versions

  System:
    OS: macOS High Sierra 10.13.6
  Binaries:
    Node: 10.9.0 - /usr/local/bin/node
    Yarn: 1.9.4 - /usr/local/bin/yarn
    npm: 6.2.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 69.0.3497.81
    Firefox: 62.0
    Safari: 11.1.2
  npmPackages:
    apollo-boost: ^0.1.3 => 0.1.15 
    react-apollo: 2.1.0 => 2.1.0 
  npmGlobalPackages:
    apollo: 1.6.0

Meanwhile, my current workaround for critical server-side queries that are not allowed to fail will be something like this:

+ const isServer = typeof window === 'undefined';

  export default ({ slug }) => (
  <Query query={BLOG_POST_QUERY} variables={{ slug }}>
      {({ data, loading, error }) => {
+     // fail the whole page (500)
+     if ((isServer && loading) || error) {
+         throw error || new Error("500: Critical query failed");
+     }

      if (loading) {
          return <span>loading...</span>;
      }

-     if (error) {
-         // fail the whole page (500)
-         throw error;
-     }

      const blogPost = data && data.blogPost;
      if (!blogPost) {
          // fail the whole page (404 - blog post not found)
          const e = new Error("Blog post not found");
          e.code = "ENOENT";
          throw e;
      }
      return <BlogPostRepresentation blogPost={blogPost} />;
      }}
  </Query>
  );

I don't see how I would render server-side 500 instead of 200 otherwise and I can also imagine that quite a few developers are not aware of what's going on 🤔

WDYT folks?

JohannesHome commented 5 years ago

This is a big issue since this messes with SEO, loading pages being index etc. Apollo is swallowing errors, even though error handling has been implemented simply because it is not being propagated when doing SSR.

It works perfectly in the client. Is there any plans on fixing this? Or on how to do this the right way with apollo?

jaydenseric commented 5 years ago

@JohannesHome Apollo Client caches the objects received from query or mutation responses by their ids (normalization); it doesn't cache metadata about the actual network requests such as HTTP status. Where would that data live, if a response gets torn into multiple separately cached pieces? A query component might render data pieced together from cache from 5 separate requests. Loading and error states are local component state that can't be serialized on the server for hydration on the client. I doubt you will be able to SSR errors with Apollo anytime soon.

Some of my design goals for graphql-react were to think fresh about cache so that errors can be server side rendered, queries to multiple GraphQL APIs can be cached in one client instance, and remove the need for interface and union fragment matching. The implementation is waaaaay simpler; with a bundle impact around 2kb min gzip vs Apollo's ~32kb.

JohannesHome commented 5 years ago

@jaydenseric what are you saying?

My question: When an error occurs that is passed through by the Query component in the client-side, shouldn't that same behaviour occur on server side?

jaydenseric commented 5 years ago

Server side rendering happens in 2 phases, firstly getDataFromTree() populates a cache by fake rendering, then that cache is used to render on the server for real. Error state that was rendered within getDataFromTree() is lost when the actual render to string happens. Even if the getDataFromTree() render could be served, it has no way to put the error states into cache to serialize and send to the client, so you would get initial HTML with the errors, but then as soon as React runs on the client it would get wiped and show loading states (SSR/client markup mismatch).

JohannesHome commented 5 years ago

Sorry english is not my native tongue. So your saying no?

jaydenseric commented 5 years ago

shouldn't that same behaviour occur on server side

No, the behavior is not exactly the same on the server as the client. Even if it was, due to the Apollo cache format it has no way to communicate that there were loading errors to the client. So the client would notice some data is missing for some components and attempt to load them, only to discover the same errors the server discovered.

JohannesHome commented 5 years ago

Looks to me like this issue is a duplicate to https://github.com/apollographql/react-apollo/issues/2134

I fixed the issue for me through catching the error from getDataFromTree() and rendering the app again with an error state that I pass in. This way the SSR error page looks the same as the page that the client would render.

tobobo commented 5 years ago

I've opened a PR to fix the docs with respect to the incorrect guidance around errorPolicy and cacheing. https://github.com/apollographql/apollo-client/pull/4237

hwillson commented 5 years ago

This should no longer be an issue with current versions of apollo-client / react-apollo. Thanks!

fenok commented 5 years ago

And yet it still is.

jacob-ebey commented 5 years ago

This is still an issue. If you need a repo case, let me know and I can get you one.

hwillson commented 5 years ago

@jacob-ebey If you can provide a repro using React Apollo 3, that would be great!

jacob-ebey commented 5 years ago

@hwillson, Here is a repro case: https://github.com/jacob-ebey/apollo-ssr-bug

Just run yarn then yarn start to get going. If you remove the "error" field from the index.ts page's query, you'll be back in a successful situation.

(fyi, if you guys are hiring, I'd love to contribute to this project in a professional capacity)

hwillson commented 5 years ago

Thanks for the reproduction @jacob-ebey! We're definitely hiring - all of our open positions are here, but the position that's most directly related to this repo is here. Thanks again!

jacob-ebey commented 5 years ago

I did a tiny bit of preliminary digging and it appears that in both ssr/getMarkupFromTree as well as hooks/ssr/consumeAndAwaitPromises have zero error handling for promises.

Adding in a concept of partial success instead of using Promise.all may be a potential solution here:

packages/hooks/src/ssr/RenderPromises.ts

public consumeAndAwaitPromises() {
  const promises: Promise<any>[] = [];
  this.queryPromises.forEach((promise, queryInstance) => {
    // Make sure we never try to call fetchData for this query document and
    // these variables again. Since the queryInstance objects change with
    // every rendering, deduplicating them by query and variables is the
    // best we can do. If a different Query component happens to have the
    // same query document and variables, it will be immediately rendered
    // by calling finish() in addQueryPromise, which could result in the
    // rendering of an unwanted loading state, but that's not nearly as bad
    // as getting stuck in an infinite rendering loop because we kept calling
    // queryInstance.fetchData for the same Query component indefinitely.
    this.lookupQueryInfo(queryInstance).seen = true;
    promises.push(promise);
  });
  this.queryPromises.clear();

  const results = await Promise.all(promises.map(p => p.catch(e => e)));
  const processed = results.reduce((p, c) => {
    if (!(c instanceof Error)) {
      return [[...p[0], c], p[1]];
    } else {
      return [p[0], [...p[1], c]];
    }
  }, [[], []]); // Returns valid results in index 0 and errors in index 1

  return processed;
}

nathanschwarz commented 4 years ago

any update on this ? I'm still having this issue

maapteh commented 4 years ago

Im having the same issue and made a nextjs reproduction in https://github.com/apollographql/react-apollo/issues/3918 I added the bug in the apollo-react part because of the treewalker and its unclear for me where the real problem is. So thats why i will reference my bug to this one as well :)

Reading the comment above https://github.com/apollographql/apollo-client/issues/3897#issuecomment-433028631 makes me think this will not be an easy fix :(

p.s. as @kachkaev is mentioning "When a request fails during getDataFromTree(), the errors are suppressed with catch", according to errorPolicy if you set it to 'all' you don't need to catch it. But you will still have same outcome, no error passed :(

eric-burel commented 4 years ago

Hi, I've struggled for a while with this bug, in the context of Next too. Still not completely sure what happens.

A as a quick solution, I use the latest version of "next-with-apollo". When an error is thrown by getDataFromTree, I get the message server-side, and the app in loading state client side. This behaviour sounds ok to me.

It's kinda disturbing at first but at least the server is showing a clear error message, so that's a sensible behaviour

But... that's where things are becoming fishy:

In some complex app, I still have this issue if a query is failing in _app. It will also render queries in underlying components in loading state. But I don't have an error thrown from my withApollo. I lack a repro at this point, still working on it.
in Apollo v3, either Apollo itself does not work, either I lose SSR. Sounds ok since it's still in beta so I'd advise to stick to v2.

samueljun commented 3 years ago

After spending a bunch of time debugging, I was able to figure out that the issue is specific to the getDataFromTree() SSR method whereas the renderToStringWithData() SRR method doesn't have the issue and behaves as expected. Here are some sandboxes demonstrating the difference on apollo client v3 (I was also able to repro on v2):

👎 getDataFromTree() method sandbox: https://codesandbox.io/s/react-apollo-ssr-error-isnt-cached-getdatafromtree-method-upgraded-to-apolloclient3-p0k1y 👍 renderToStringWithData() method sandbox: https://codesandbox.io/s/react-apollo-ssr-error-isnt-cached-rendertostringwithdata-method-upgraded-to-apolloclient3-ohktp

getDataFromTree()

The getDataFromTree() method doesn't have consistent behavior with errors because it doesn't store errors in the apollo cache to be available for the final markup generation method (React's renderToString()).

Essentially the getDataFromTree() function does two render passes. The first pass is to fetch data via executing all queries in the render tree, and the second pass to close out all the executing queries.
Once getDataFromTree() is done executing, we then need to run React's renderToString() to generate the actual markup. You can see that there are 3 console.logs in the sandbox for each render pass (2 from getDataFromTree, 1 from renderToString).
In the success case, the reason that renderToString() is able to have the hydrated data from getDataFromTree() is because the apollo cache is hydrated with the success responses.
For the error case however, the apollo cache doesn't store errors for whatever reason. So when React's renderToString() method runs, the apollo react client doesn't see the error and just returns a loading == true. This then produces the inconsistent server-side vs client-side situation that we're seeing.

renderToStringWithData()

This is in contrast with the renderToStringWithData() method which works as expected. This is because renderToStringWithData() generates the markup right within the context of the second pass where the error is still in memory. You can see that there are only 2 console.logs in the sandbox for just the two passes required for executing the queries.

It looks like renderToStringWithData() is the more correct/consistent method for SSR until the apollo cache starts caching error for getDataFromTree() method to work as expected.

bignimbus commented 1 year ago

Thanks all for your patience on this issue. The team is prioritizing addressing SSR use cases, see https://github.com/apollographql/apollo-client/issues/10231 and feel free to leave feedback!

apollographql / apollo-client

Errors during SSR are rendered as "loading" #3897