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

ApolloCall execute / toFlow / exception handling improvements #4003

Closed
Tracked by #4171
BoD opened this issue Apr 7, 2022 · 1 comment
Closed
Tracked by #4171

ApolloCall execute / toFlow / exception handling improvements #4003

BoD opened this issue Apr 7, 2022 · 1 comment
Assignees
Labels
✔️ Fixed in SNAPSHOTs The fix has been merged and is available in SNAPSHOTs, and will be available in the next release 🚀 Type: Enhancement ⚠️ Breaking Change To be done in next major version

Comments

@BoD
Copy link
Contributor

BoD commented Apr 7, 2022

Current situation

ApolloCall exposes 2 APIs:

  • suspend execute(): ApolloResponse
  • toFlow(): Flow<ApolloResponse>

execute() will throw an ApolloException in case of a network, http, or cache error (can also be ApolloCompositeException in case of network and cache errors)

It will also throw an exception if the underlying Flow contains more than 1 element (ex: CacheAndNetwork or with @defer)

Users need to handle 2 kinds of errors:

  • network/http errors with a try catch
  • GraphQL errors by looking at .errors

(With ApolloResponse.dataAssertNoErrors this can be centralized in one try catch)

toFlow() will throw an exception either as soon as it is encountered, or assembled in ApolloCompositeException after some responses have been emitted, depending on the case.

Cache misses are either emitted with a null data or not emitted at all (configurable with emitCacheMisses).

Suggested change

  • Add an exception: ApolloException? field to ApolloResponse
  • Never throw ApolloException in toFlow() : instead, ApolloResponse are emitted with a null data and non-null exception
  • To be discussed: should we also not throw in execute()? Not sure of the most logical behavior to adopt with CacheFirst and a cache miss + network error for instance. No exception thrown but return an ApolloResponse with a null data and an ApolloCompositeException in exception?

Benefits:

  • Handling network/http errors and GraphQL errors is done in a similar way: look at exception or errors
  • More predictable behavior of toFlow(): cache misses / network errors can always be emitted, the caller can decide to ignore them or not by looking at exception, depending on the need
  • This can potentially simplify our code in some places

More info

  • This would be a breaking change.
  • A comment that prompted this idea is here.
@BoD BoD added 🚀 Type: Enhancement ⚠️ Breaking Change To be done in next major version labels Apr 7, 2022
@BoD BoD mentioned this issue Nov 3, 2022
33 tasks
@BoD BoD mentioned this issue Feb 14, 2023
4 tasks
@BoD BoD self-assigned this Feb 16, 2023
@martinbonnin martinbonnin added the ✔️ Fixed in SNAPSHOTs The fix has been merged and is available in SNAPSHOTs, and will be available in the next release label Mar 2, 2023
@martinbonnin
Copy link
Contributor

This is now merged in main. It's still time to make adjustments if needed though. Please go to the RFC for comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✔️ Fixed in SNAPSHOTs The fix has been merged and is available in SNAPSHOTs, and will be available in the next release 🚀 Type: Enhancement ⚠️ Breaking Change To be done in next major version
Projects
None yet
Development

No branches or pull requests

2 participants