Add bookmarks to Wear app #549
Conversation
| apolloClientCache.getClient(conference, uid).query(GetBookmarksQuery()) | ||
| .tokenProvider(tokenProvider) | ||
| .fetchPolicy(fetchPolicy) | ||
| .execute() | ||
| .toFlow() |
There was a problem hiding this comment.
This can't be execute if we take fetchPolicy in as a parameter.
There was a problem hiding this comment.
Agreed. execute() is a bit awkward because of CacheAndNetwork that emits twice. There is a bit of brain muscle around it though so it's hard to change at this point.
There was a problem hiding this comment.
I might add a simple one so it's hidden from clients.
There was a problem hiding this comment.
Probably an extension function to ApolloCall?
There was a problem hiding this comment.
Not sure I follow. Soemthing like this?
ApolloCall.cacheAndNework()
There was a problem hiding this comment.
Not sure, I think I want ApolloCall.asStateFlow(fetchPolicy): StateFlow<Result<Data>> that hides all errors when it has any results to show. And a failed form if both network and cache failed.
There was a problem hiding this comment.
Hiding errors is a bit risky. There's some background there where we did that for .watch() and some users required access to the individual errors.
There was a problem hiding this comment.
What about Flow but it doesn't go from a a non null data, to null?
There was a problem hiding this comment.
Maybe yea. In all cases, I'd proceed with caution there as we've been burnt by this before (one instance there). Maybe keep the current toFlow() that emits everything and add helper functions on top of it. If it's StateFlow it can be one layer of abstraction above or so.
| @@ -150,11 +150,15 @@ class ConfettiRepository : KoinComponent { | |||
| } | |||
| } | |||
|
|
|||
| suspend fun sessionDetails( | |||
| fun sessionDetails( | |||
There was a problem hiding this comment.
It feels like we don't have a common plan for how to implement these. A similar issue happens with Room, for suspend functions vs Flow.
But especially here, it would be nice if we could hide away differences between CacheFirst, CandAndNetwork, NetworkFirst.
I'm unclear what happens when CacheAndNetwork, gets a valid cache result, but then fails on network. Does it silently drop it, so the cache result stands? Or show an error?
There was a problem hiding this comment.
what happens when CacheAndNetwork, gets a valid cache result, but then fails on network
It emits 2 ApolloResponse:
- First one with
apolloResponse.exception == null - Second one with
apolloResponse.exception != null
There was a problem hiding this comment.
What about the data?
Should we ignore that second one? assume it's just network is down?
There was a problem hiding this comment.
What about the data?
- First one with
apolloResponse.data != null - Second one with
apolloResponse.data == null
Should we ignore that second one? assume it's just network is down?
I would ignore it yes. And rely on pull to refresh if the user really want latest data.
To me:
- initial load with
CacheAndNetworkmeans: display something as soon as possible, update in the background - pull to refresh means: I always want fresh network results (or an error else)
The more I think about it, the less I like CacheAndNetwork though because it updates the data without user action which might feel a bit weird.
There was a problem hiding this comment.
The more I think about it, the less I like
CacheAndNetworkthough because it updates the data without user action which might feel a bit weird.
I like that, but it's a change for mobile.
| @@ -27,7 +27,7 @@ class SpeakerDetailsViewModel( | |||
|
|
|||
| val speaker: StateFlow<UiState> = flow { | |||
| // FixMe: add .speaker(id) | |||
| val response = repository.conferenceData(conference = conference, FetchPolicy.CacheOnly) | |||
| val response = repository.conferenceData(conference = conference, FetchPolicy.CacheFirst) | |||
There was a problem hiding this comment.
CacheFirst seems safer and more like what we want than CacheOnly.
There was a problem hiding this comment.
+1. That'll fix a deeplink to a speaker details if the conference has never been opened before.
Also we should get only the speaker instead of retrieving all the conference data (I can do that in a follow up PR)
There was a problem hiding this comment.
On the pages that are information focused, instead of action focused. I decided to make this more muted.
chipType = StandardChipType.Secondary
No description provided.