Skip to content
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

ObservableQuery.refetch() returns rejected Promise instead of throwing when fetchPolicy === 'cache-only' #1592

Merged
merged 2 commits into from
Apr 19, 2017

Conversation

BlooJeans
Copy link
Contributor

@BlooJeans BlooJeans commented Apr 14, 2017

Problem / Situation
At the moment react-apollo (v1.0.1) sets unmounted components to cache-only (apollographql/react-apollo#622) and apollo-client will throw an error when calling refetch() on a query that is set to cache-only.

In our app, this was having the effect of force closing during a pull to refresh / refresh on resume because we didn't guard against refetch() possibly throwing an error (but we did handle a failed refetch promise via .catch()).

This is a somewhat controversial code-style question, so I'll write out my reasoning -- If you/y'all disagree with my argument, feel free to close --

It seems wrong to me to throw an error instead of returning a rejected Promise when an internal state is misconfigured. For other cases like fetchMore(fetchMoreOptions), it makes sense to throw an error because it's a developer-fault configuration error ("you're using this wrong"). The method will fail as soon as it's executed it because the usage of it is wrong and the developer can then fix the arguments. For refetch, however, it throws an error when the ObservableQuery has a misconfigured internal variable, which can (as shown in the react-apollo issue) change after the fact.

Furthermore, by returning a promise, the developer should already know to handle the failure case in the .catch(). Without this PR, the developer needs to wrap every refetch() in a try/catch in addition to their .catch() on the off chance that a refetch might crash their app :)

@BlooJeans BlooJeans force-pushed the refetch-doesnt-throw branch from c7810db to 709029c Compare April 14, 2017 20:20
@helfer
Copy link
Contributor

helfer commented Apr 19, 2017

Agreed! Thanks for the PR @BlooJeans, looks good to me! It's technically a breaking change, but I think a patch version will have to do here, since it's patching a known problem.

@helfer helfer merged commit e82336e into apollographql:master Apr 19, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants