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

getDataFromTree throws errors when any query fails #615

Closed
choonkending opened this Issue Apr 10, 2017 · 33 comments

Comments

Projects
None yet
@choonkending

choonkending commented Apr 10, 2017

Background

I asked a question on StackOverflow today here.

I thunk about it harder afterwards and decided that this is a discussion worthy of a Git issue.

Problem

Any error gets thrown in getDataFromTree.

Expected/Proposed Behaviour

Return the data with an error Array, without throwing an error.

Why?

Our GraphQL server speaks to multiple APIs and returns a graphQL response which is handled by Apollo UI.

Certain GraphQL errors are not considered critical, and we want to still render a page with it. E.g.

data: {
   thing: {
       id: "1234"
       name: "Thing"
       nonCriticalData: null
   },
   error: {
       graphQLErrors: [
           path: ['thing', 'nonCriticalData']
       ]
   }

With the above response we get from apollo, we want to still render a page with thing.

I have followed #488 (comment) and got to this point.

In my mind the errors should be returned and not thrown unless necessary, because not all errors are equal.

Ideally I would like to do the following

getDataFromTree(app)
    .then(() => {
             const content = ReactDOMServer.renderToString(app);
             const { data } = client.getInitialState();   <------ Data (with errors)
             // check for expected errors and render
             return renderPage(data);
     })
    .catch(error => {
         // handle unexpected errors
    });

Version

  • apollo-client@1.0.0-rc2
  • react-apollo@1.0.1

P.S. Thanks for an awesome library. Happy to help out if needed.

@helfer

This comment has been minimized.

Member

helfer commented Apr 21, 2017

Hey @choonkending, sorry it took me so long to get around this issue. I agree with you 100% that this behavior isn't consistent, but because of the way server side rendering is implemented, we can't easily change it to do what you want. The reason is that the queries are all executed in a first pass on the route, then their data is stored in the cache. When you call renderToStringWithData, the data from the cache is used to render the tree synchronously. We don't cache errors, so there's currently no easy way to change this without either changing the way server side rendering works (make it asynchronous), or changing the way errors are handled in the store. Even if the rendering were identical on the server and the client given the same query, the initial react render on the client would be unaware of the errors (because they're not stored), so there would be an inconsistency there. 🙁

We're planning improvements in the next version of the API that would make this problem go away, but for the moment I think it's quite difficult to change.

If you're very interested and motivated to work on this problem, just let me know, we could definitely use the help!

@choonkending

This comment has been minimized.

choonkending commented Apr 21, 2017

@helfer No worries! Thanks for getting back to us.

If you're very interested and motivated to work on this problem, just let me know, we could definitely use the help!

Definitely.

Just to get a better understanding:

  • How does the caching model work? Can you point us to parts of the codebase where you think this is the tricky part to change?
  • Are there any docs/git issues around the thinking for the next version of the API?
@helfer

This comment has been minimized.

Member

helfer commented Apr 21, 2017

@choonkending the cache normalizes all data, and when you run a query against the cache, the result is assembled from the normalized parts. That's a problem if errors don't have paths, because then there's no way to know which field an error is associated with, and no way to know if an error should be returned with the query run against the cache.

There's no discussion issue yet for the next version of the API, but I'm trying to put an initial post together soon. In the meantime, I think the best thing to do would be to write a use-case for each requirement that we can think of. Having SSR be consistent with client rendering is one use-case, and I think we should write that down and document it well, so we can have a discussion around it.

For that purpose, could you add a comment here, please? #519

You should also feel free to write down other use-cases that haven't been mentioned yet (so far there are very few, I'm hoping people will add more to it, even current functionality of react-apollo that they are relying on).

@jbaxleyiii

This comment has been minimized.

Member

jbaxleyiii commented May 3, 2017

This really needs to be talked through and fixed 💪 After I clean up the tests and add better typings. I'll try to get around to error handling and SSR!

@Dattaya

This comment has been minimized.

Dattaya commented Jun 30, 2017

Just want to leave a comment in support of @choonkending proposal. I already handle errors in components that perform queries: if {props.data.error} return a component that shows the error's message (I'm talking about errors that are thrown from resolvers/connectors). So I expect the server to render the exact result that would be rendered on the client. I tried to use a workaround suggested in #488 (comment) but it doesn't really work, because apolloClient.getInitialState() returns a data object without an error.

@OllieJennings

This comment has been minimized.

OllieJennings commented Jul 4, 2017

I would just like to say that this has become a big issue for me.

As my app has gotten more complex, i have gone down the route of having 2 bundles, and if a 404 then rendering 2nd bundle, and losing the SPA benefits.

If you could still keep query errors in state, and allow SSR of 404 pages, it would simply things massively

@edoroshenko

This comment has been minimized.

edoroshenko commented Jul 17, 2017

Hi @helfer

If you're very interested and motivated to work on this problem, just let me know, we could definitely use the help!

I'm very interested and motivated (as long as it's my impediment).
Could you let me know if you already started approaching this problem? If no, do you have some thoughts how you would approach it?

@helfer

This comment has been minimized.

Member

helfer commented Jul 17, 2017

Hi @edoroshenko. That's awesome. @jbaxleyiii can probably tell you more about it since he designed it together with Tom and is now working on Apollo full time!

@edoroshenko

This comment has been minimized.

edoroshenko commented Jul 17, 2017

Thanks @helfer!
Do you guys have a slack channel or what is the best way to communicate?

@jbaxleyiii

This comment has been minimized.

Member

jbaxleyiii commented Jul 17, 2017

@edoroshenko you bet! Check out http://dev.apollodata.com/community/ and join us on slack! I'd be happy to work together on finally bringing some better error support to Apollo and SSR in general!

@jbaxleyiii jbaxleyiii self-assigned this Jul 17, 2017

@edoroshenko

This comment has been minimized.

edoroshenko commented Jul 18, 2017

The way I see the solution:

  1. Start storing GraphQL errors in the store
  2. Stop throwing GraphQL errors from apollo client
  3. Introduce error-policy option, that would define what to do with errors in the cache. It could be either refetch or reuse. refetch is a default one and should work as it works now. So apollo should always do query even if there's an error in the cache. reuse should tell apollo client to respond with an error if it has one in the cache.
  4. In graphql high order component we should always use reuse error policy first time and refetch on any subsequent request. This way we'll be able to transfer the state with errors from server to client and perform first client-side rendering based on it without extra queries to the server.

This plan will affect apollo-client and react-apollo repos.

@jbaxleyiii , @helfer, what do you think about this plan?

Or do you have another ideas, how to approach this issue?

@stale stale bot added the no-recent-activity label Aug 10, 2017

@stale

This comment has been minimized.

stale bot commented Aug 10, 2017

This issue has been automatically labled because it has not had recent activity. If you have not received a response from anyone, please mention the repository maintainer (most likely @jbaxleyiii). It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

@stale

This comment has been minimized.

stale bot commented Aug 24, 2017

This issue has been automatically closed because it has not had recent activity after being marked as no recent activyt. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to React Apollo!

@ghostganz

This comment has been minimized.

ghostganz commented Sep 11, 2017

This is still a huge issue for us, how do I re-open it?

We've wasted a lot of time trying to figure out why our error handling doesn't work on SSR while it works fine in the browser. It should be documented somewhere that it isn't expected to work yet!

@t0x1c123

This comment has been minimized.

t0x1c123 commented Sep 24, 2017

Same problem here. Any news? For example i want to throw error with info when user is not authenticated. This doesn't work with SSR getDataFromTree 😢

@OllieJennings

This comment has been minimized.

OllieJennings commented Sep 26, 2017

@jbaxleyiii any news on this since apollo 2.0 is coming, would be nice to see something like this sorted

@gabriel-miranda

This comment has been minimized.

gabriel-miranda commented Dec 10, 2017

This is still happening and it kinda breaks SSR.
when query works:
getDataFromTree => data = {loading: false, error: false, <key>: ...}
when query fails:
getDataFromTree => error thrown and all data discarded {error}
This makes that after SSR there is no error, loading is set to true and every <key> needed is empty.
I think it should work kinda like this
getDataFromTree => data = {loading: false, error: true, <key>: ...}
Because if it doesnt, the whole app lose consistency.

@edorivai

This comment has been minimized.

Contributor

edorivai commented Feb 2, 2018

@jbaxleyiii Can this issue please be re-opened?

@tspecht

This comment has been minimized.

tspecht commented Feb 21, 2018

Any news on this? Having the same issue with react-apollo 2.0

@choonkending

This comment has been minimized.

choonkending commented Feb 21, 2018

Hey peeps, just giving an update of the approach we've taken in my team since this issue was created. Hopefully it helps if it applies to your use case!

We opted to rely less on react-apollo when it came to data fetching. (I'm not bagging the good work that's been done on react-apollo! It's just the way we decided to go from a technical perspective)

  1. Use ApolloClient.query in ApolloClient instead
  • This works if you have your query defined beforehand
    👍 You can fetch data first before rendering the app altogether
    👍 You can decouple your data fetching from your view altogether. Keeping them separate helped us reason about the data flow better.
  1. Use errorPolicy: all in errorPolicies
    👍 Returns data + errors

You should be able to handle server side rendering with just apollo client!

I hope this helps!

@amannn

This comment has been minimized.

Contributor

amannn commented Mar 28, 2018

I'm having the same problem. My ideal scenario would be if getDataFromTree would fill the cache and afterwards I can choose if I want to render the app with the existing cache (e.g. rendering errors or something smarter if components have errorPolicy configured) or not render the app at all (current situation).

@jbaxleyiii Based on the involvement of the community in this thread, would you mind reopening this issue?

@adamesque

This comment has been minimized.

adamesque commented Apr 24, 2018

Re: point 2 on @choonkending's comment above:

Use errorPolicy: all in errorPolicies
👍 Returns data + errors

I don't think this is working correctly currently. See #1781

@sachee

This comment has been minimized.

sachee commented Apr 26, 2018

Any update on this? This is a huge problem as it causes an entire page to error if a single query fails.

Our only other option is to basically not handle errors at all.

@realalexhomer

This comment has been minimized.

realalexhomer commented Apr 26, 2018

@joekrill

This comment has been minimized.

joekrill commented Apr 26, 2018

This is a problem for me, too. As a sort-of temporary workaround I've been experimenting with catching errors in my resolvers and adding a custom error field to pass the data down as "regular data".

Basically I have a GraphQL Error type:

type Error {
  type: String
  message: String
}

And I can add that to types as needed, based mostly on the underlying queries:

type Widget {
  id: String
  error: Error
  name: String
  description: String
}

# etc...

type Query {
  widget(id: String): Widget
}

Then in my resolver:

const widgetResolvers = {
  Query: {
    widget(_, { id }) {
      try { 
        return Widget.find(id);
      } catch(err) {
        return { 
          id,
          error: {
            type: 'UNKNOWN', // Or something meaningful
            message: err.message, // Maybe display this client-side?
          }
        }
      }
    }
  }
}

Then in my component I can check myQuery.widget.error (instead of myQuery.error, which would be the case with the built-in error handling) to determine if there is some sort of error. I think you actually would want to check for both error fields and handle either or.

It's not ideal. And it requires wrapper types for certain queries. For example, instead of being able to just return an array:

type Query {
  allWidgets(): [Widget],
}

I've got to do something like:

type WidgetCollection {
  error: Error,
  widgets: [Widget]
}

type Query {
  allWidgets(): WidgetCollection,
}

I haven't gotten too deep with this yet, so I'm not sure if this is going to be a viable workaround or not. But I'm not sure how else to handle it until there is a proper fix. Perhaps ApolloLink could be used to inject the error data some how?

@theasia-Ashish

This comment has been minimized.

theasia-Ashish commented May 9, 2018

Still same error on renderToStringWithData error data can't reach to components on SSR. If i apply errorPolicy: all, i'm getting error undefined any updates?

@theasia-Ashish

This comment has been minimized.

theasia-Ashish commented May 9, 2018

@migueloller

This comment has been minimized.

migueloller commented Jul 27, 2018

Given that this is still around, this issue should definitely be reopened.

@jamesgallagher432

This comment has been minimized.

jamesgallagher432 commented Oct 16, 2018

This issue should be reopened. I am using a similar setup as described in the question and every time there is an error, the .catch() statement logs the error to the console and stops the entire page from rendering. Any help would be appreciated.

@mhaagens

This comment has been minimized.

mhaagens commented Nov 4, 2018

Here's one approach to dealing with this. Just ignore errors on the server, but make sure you handle them in your components;

  // Ignore errors
  const errorLink = onError(({ graphQLErrors, networkError, response, operation, forward }) => {
    return forward(operation);
  });

  // Create Apollo client and link to schema
  const client = new ApolloClient({
    ssrMode: true,
    cache,
    link: ApolloLink.from([
      errorLink,
      stateLink,
      new SchemaLink({
        schema,
        context: { req, res },
        rootValue: {
          currentUser: req.user ? req.user : null
        }
      })
    ])
  });
@richardscarrott

This comment has been minimized.

richardscarrott commented Nov 6, 2018

@mhaagens but how do you handle the errors in your components?

@mhaagens

This comment has been minimized.

mhaagens commented Nov 6, 2018

If the app errors on the server I add the error to apollo local state in a second pass from a try/catch handler and then render a generic error component in the app. I can then pick up that error state on the client as well. If it errors on the client I render the same component with the client side error instead.

Basic flow;

  1. Try to render app
  2. App errors - catch it in a try/catch
  3. Run a second pass with the error passed in to local state
  4. Render error component in app

Now make sure you don't follow this example to the letter in production, you don't want to leak those errors to the client, but you can use the same idea.

SSR Route handler;

export default async (req, res) => {
  try {
    // Render response from React app
    const response = await createResponse(req, res);
    res.send(response);
  } catch (e) {
    try {
      // If errored run second pass to get React app error response
      const serverContext = { error: JSON.stringify(e) };
      const response = await createResponse(req, res, serverContext);
      res.send(response);
    } catch (e) {
      // Couldn't render error response with React
      res.send("Something went very very wrong. Failed to catch error.");
    }
  }
};

Top level app component;

{this.state.errors || this.props.clientState.serverError ? (
    <ServerError clientErrors={this.state.errors} serverError={this.props.clientState.serverError} />
) : <App />}
@osamu38

This comment has been minimized.

osamu38 commented Dec 4, 2018

any update?

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