Apollo-client: readQuery crashes if there is nothing in the store

[kemalcany]

Intended outcome:

I want to check whether or not the apollo cache(store) has a watchQuery registered for a query name (ex: get_comments). I want to use readQuery (or a similar method?) to check the store and if there is nothing, I will get an undefined or null, or -1 etc. Something to indicate that there s no watchQuery exists under that query name.

Actual outcome:

When I try to read something from cache via readQuery, the app crashes if the query doesn't match. The error I get is below:

ERROR Error: Uncaught (in promise): Error: Can't find field collections on object (ROOT_QUERY) undefined.

I can't catch this error, since the return of the readQuery is not a promise or observable bur just a simple object. This error continues even if I convert the return to Observable via Observable.of(...readQuery())

How to reproduce the issue:

Just try to readQuery a random query name (ex: xwyz_xxx) and you should be able to get the error (which I couldn't managed to catch before it crashes the app)

Please let me know if I am missing something completely. I am not very experienced in this repo. Thanks for the help.

[helfer]

Thanks @kemalcany, this looks like a pretty obvious bug! It would help a lot if you could make a PR with a failing test. Should be pretty easy, do you want to give it a go?

[kemalcany]

Thanks for the quick response @helfer . You guys are really doing a great job!

I've dug a little further and found a really nice error message getting thrown by src/data/readFromStore

The return from readQuery is not a promise or observable but I think that's by design. (and it works well)
But wrapping the readQuery around a try/catch block solves the problem.

Whether not being able to find a query ID in the store should cause an error or not is a question of design I suppose. Could it be nicer if the return was a simple string or int like -1 (kind of like Array.indexOf) ?

Otherwise I don't think this is a bug and could be closed.

Thanks for the help!

[jfalk]

The graphql function seems to be running into the same issue as above when it calls executeField.

[cesarsolorzano]

@kemalcany Apollo actually uses a try/catch block in order to prevent this. So, I'm closing this since this is the current intended behavior.

I'll let @helfer discuss the design question and therefore to reopen it if that's the case.

[cusxio]

@helfer, @cesarsolorzano

readQuery throws an error whenever the cache/store is empty as mentioned here.

However, I have a use case where I need to conditionally check if the cache exists in the store, if it exists, update it via writeQuery, if not, ignore it.

because readQuery throws an error when the query fails, I need to wrap it in a try/catch block to _silent_ it.

Is this the right way? Or is there another way to conditionally check if the cache exists in the store?

Here's a snippet of what I'm currently doing:

graphql(DELETE_PARENT_MUTATION, {
  props: ({ mutate, ownProps: { topic } }) => ({
    deleteParent: variables =>
      mutate({
        variables: { ...variables },
        update: (proxy, { data }) => {
          const deletedTopicId = data.deleteTopicById.topic.id;
          const parentTopicId = topic.parentTopic.id;

          try {
            const prev = proxy.readQuery({
              query: TOPICSPAGE_QUERY,
              variables: { topicId: parentTopicId }
            });
            proxy.writeQuery({
              query: TOPICSPAGE_QUERY,
              data: update(prev, {
                topic: {
                  childrenTopics: {
                    edges: {
                      $apply: edges => edges.filter(({ node }) => node.id !== deletedTopicId)
                    }
                  }
                }
              })
            });
          } catch (e) {} // eslint-disable-line
        }
      })
  })
})

[abhiaiyer91]

Would it be alright if readQuery returned null with the helpful error message? Am i the only one not a fan of try/catching all my readQuerys?

[helfer]

I think @cesarsolorzano is right here. Since this isn't a promise API, throwing an error is the logical thing to do if the data you asked for isn't there. I agree that it would be a nicer API if we could just return null and a list of errors, but that would be a breaking change which we can't practically do until 2.0.

[matthewerwin]

@helfer the design of this to be an error state seems suspect. Particularly since there is no guarantee that the cached data would exist for a related query at the time say a mutation attempts to update records related to it which may only load on certain pages of a website. A site of any complexity is going to regularly run into this all the time because there is no predictability that any related queries have been cached yet.

Unwinding stacks or silencing errors (which may unintendedly silence more than the target error) is expensive and issue-prone. This problem could be solved without creating a breaking change by either adding a new method such as hasQuery() or by adding in an option on DataProxyReadQueryOptions like throwErrorOnNotFound=false to retain existing behavior but provide the option to avoid try/catch & wasteful stack unwinding or internal error handling logic for this condition.

[raysuelzer]

@matthewerwin -- I fully support This problem could be solved without creating a breaking change by either adding a new method such as hasQuery() I have never run into a cache implementation that doesn't have a method for checking the existence of an item...

[jonasdumas]

+1
I have the same problem.
I've a list of item (which launch the main query) with an add button . The add button drive to a form where it's possible to add an item (mutation with query update). If the user follow this path it works such as the main query was previously queried and is present in the cache.
But, if the User goes directly to the Add form without passing to the list of items it crashes as the store in empty. Such story path is very basic and common.

Of course, i can add this try/catch around the update function. But it's very verbose and easy to forget. It would be much cleaner to have one of the following features:
1) As @helfer said, a simple NULL return if the cache is empty for a given query.
if it break the main Apollo coding rules (such as it's not a promise as mentioned by @helfer), then:
2) a function to check if a query exist in the cache or not as @matthewerwin suggested
3) Or, a "tryUpdate/skipError" boolean parameters to control how should react the update in case of error.

[jonasdumas]

I didn't see @matthewerwin comment who was saying exactly what I just comment. So +1 for his remark. I would love to have this feature.
Thank you very much at all the Apollo team for your awesome work. It's so playful and useful to work with your library.

[rpedroni]

Hey guys, any update on this? I really _dislike_ writing all these try / catch :(

[Ryskulov]

now 2.1 version still need use try/catch ?

[wuzhuzhu]

@Ryskulov I'm afraid is true.

[bbeldame]

I think this issue should be open again

We should be able to have another options apart form variables and query, something like : skipError (boolean)

[masterkidan]

Performance of try/catch in js is still expensive https://jsperf.com/try-catch-performance-jls/2
Specifically, when the error condition is being hit.
Can you please try adding in either an option to skip the error and return, or alternatively return undefined instead?

[bbugh]

This is still an issue for sure. It looks like one of the apollo team migrated an existing issue for it to their new suggestions issue tracker - https://github.com/apollographql/apollo-feature-requests/issues/1 Maybe it will get more traction there.

[joshuajhun]

I'm with @rpedroni and @matthewerwin. It would be super nice to have to use a try/catch and that hasQuery() function would be amazing.

[richardsengers]

This should indeed be re-opened, took me quite a while why I was getting an error, but sometimes a query isn't in the cache yet.

[sunwukonga]

It's apollo-cache-inmemory/lib/readFromStore.js -> diffQueryAgainstStore() that throws errors if there are missing fields. I think that there are two categories of missing fields.

1. Missing fields that are a result of mismatched fragments that access the same cache data, i.e. trying to grab fields that were never pulled with the original network access [definitely deserves a thrown error, as this is probably a programmer error], and
2. The cache signature is missing from the store entirely, i.e. it has not been initialized [this should return null, not throw an error].

It seems to me that everyone would be relatively mollified if diffQueryAgainstStore() did not throw an error for the case where the query doesn't exist in the cache yet.

[gomesalexandre]

What's the status on this ? try/catch isn't elegant in most cases. and is quite expensive

[Stefano1990]

I guess this is still an issue.

[matthewerwin]

I haven't been able to find the time to write up something on this for a general PR while running our business. We ended up just patching it internally. Without a doubt it should never be the case that exception-based handling is part of a very standard use case. I haven't looked at this in the latest versions of Apollo.

This ticket has been closed in favor of a feature request:

https://github.com/apollographql/apollo-feature-requests/issues/1

[Risbot]

you can write typePolicy and return null when data is not in store, like this:

 new InMemoryCache({
    typePolicies: {
      Query: {
        fields: {
          fieldName: {
            read: fieldName => {
              return fieldName || null;
            },
          },
        },
      },
    },
  });