DO NOT MERGE - WIP Make fetcher a class to avoid the API issues #139

yigit · 2020-03-23T01:25:06Z

This CL changes Fetcher to be a function that returns value/error instead of
throwing. Internally, RealStore always uses it and any exception thrown in
such function will escalate.

There are builders in Fetcher.** which can receive different fetcher functions.

This is an alternative implementation to support fetchers that return values
and not throw exceptions.

Please see our contributing guidelines (contributing.md) primarily make sure to sign our cla as we cannot accept code externally without a signed cla

https://opensource.dropbox.com/cla/

This CL changes Fetcher to be a function that returns value/error instead of throwing. Internally, RealStore always uses it and any exception thrown in such function will escalate. There are builders in Fetcher.** which can receive different fetcher functions. This is an alternative implementation to support fetchers that return values and not throw exceptions.

eyalgu · 2020-03-30T15:02:05Z

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

+    companion object {
+        fun <Key, Output> fromValueFetcher(
+            doFetch: (key: Key) -> Flow<Output>
+        ): Fetcher<Key, Output> = FlowingValueFetcher(doFetch)
+
+        fun <Key, Output> fromNonFlowingValueFetcher(
+            doFetch: suspend (key: Key) -> Output
+        ): Fetcher<Key, Output> = NonFlowingValueFetcher(doFetch)
+
+        fun <Key, Output> fromNonFlowingFetcher(
+            doFetch: suspend (key: Key) -> FetcherResponse<Output>
+        ): Fetcher<Key, Output> = NonFlowingFetcher(doFetch)
+    }
+}


This would simplify the builder indeed (taking the number of builder create function from 8 to 2 in exchange for adding 4 entry points for the fetcher). I would add some builder extension functions to make the common case easy though.

I would like to keep the fetch and transform as separate functions though. The reasons are:

The user already needs to transform their internal library results to FetcherResult so it's no additional burden

we can enforce that the transform is not a suspend function (it should be quick, the heavy operations should be in the fetcher)

less code to maintain (but we can add an extension function if we want the other api back)

How about something like this?
https://gist.github.com/eyalgu/3d03420f72bc0abed76f88be111ce6d2

this looks good to me

Hmm i'm not sure how much we benefit from doUnwrap being explicit. It is all part of user's code so whether is a suspend or not seems unrelated (re: reason 2)? Maybe i'm not fully getting what you mean there.
In fact, i think it would be more annoyance in terms of API if they needed to provide two separate functions for something they could be just handling within their function.

digitalbuddha

I like the direction this is going as it adds functionality while keeping the default case fairly simple. Whenever you have time I'd love to see this cleanup up into a real impl (no rush as this is external request)

yigit

one thing i don't like in this CL is that we have the ResponseOrigin.Fetcher constant along with the Fetcher interface now. idk if we care though, just something that required me to change tests.

yigit · 2020-04-18T23:41:40Z

store/src/main/java/com/dropbox/android/external/store4/StoreBuilder.kt

         * @param fetcher a function for fetching network records.
         */
        @OptIn(ExperimentalTime::class)
+        @Deprecated(message = "Creating a flow from a function is deprecated, use Fetcher",


eyal@ is this the one you want to keep instead of deprecating ?

yigit · 2020-04-18T23:41:54Z

store/src/main/java/com/dropbox/android/external/store4/StoreBuilder.kt

         * @param fetcher a function for fetching a flow of network records.
         */
        @OptIn(ExperimentalTime::class)
+        @Deprecated(message = "Creating a flow from a function is deprecated, use Fetcher",


this as well. cc: @eyalgu

digitalbuddha · 2020-04-19T23:59:36Z

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

+         * Creates a [Fetcher] from the given function. If it throws an error, the response will be
+         * wrapped in a [FetcherResponse.Error].
+         */
+        fun <Key, Output> fromNonFlowingValueFetcher(


nit: fromNonFlowingResponseFetcher sounds better imo

digitalbuddha · 2020-04-20T00:01:29Z

store/src/main/java/com/dropbox/android/external/store4/FetcherResponse.kt

+     */
+    // TODO support non-throwable errors ?
+    class Error<T>(
+        val error: Throwable


what is the cost of supporting non throwable errors now

eyalgu

I've integrated this with #123. Lets land it there or the rebase will be impossible

* Support non exception errors from fetcher * revert parital changes to store builder to reduce noise * finish off diff * Allow to create a FetcherResult.Error without a Throwable. Add tests * Add missing funcion and more tests * lint * unflake RxFlowableStoreTest * try to rename FakeFetcher to FakeRxFetcher to (maybe) solve missing codcov * move SourceOfTruth out of impl package * Rename accidental change of RxStoreBuilder.fromMaybe back to formSingle * Introduce Fetcher from #139 * fix Rx artifact * delete legacy presistor factory * fix api file * move fetcher to be a typealias * code review comments + clean up documentation * code review comments * Update store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt Co-Authored-By: Yigit Boyar <[email protected]> * Revert "Update sample app's build.gradle to refer to the externally released version of Store (#159)" This reverts commit fc8da86. * update releasing.md Co-authored-by: Yigit Boyar <[email protected]>

eyalgu · 2020-05-04T23:05:32Z

@yigit can I close?

* Support non exception errors from fetcher * revert parital changes to store builder to reduce noise * finish off diff * Allow to create a FetcherResult.Error without a Throwable. Add tests * Add missing funcion and more tests * lint * unflake RxFlowableStoreTest * try to rename FakeFetcher to FakeRxFetcher to (maybe) solve missing codcov * move SourceOfTruth out of impl package * Rename accidental change of RxStoreBuilder.fromMaybe back to formSingle * Introduce Fetcher from MobileNativeFoundation#139 * fix Rx artifact * delete legacy presistor factory * fix api file * move fetcher to be a typealias * code review comments + clean up documentation * code review comments * Update store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt Co-Authored-By: Yigit Boyar <[email protected]> * Revert "Update sample app's build.gradle to refer to the externally released version of Store (MobileNativeFoundation#159)" This reverts commit fc8da86. * update releasing.md Co-authored-by: Yigit Boyar <[email protected]>

yigit force-pushed the yigit/errors-as-values branch from 3a6e0d6 to 6234efc Compare March 23, 2020 01:35

yigit mentioned this pull request Mar 23, 2020

Support non exception errors from fetcher #123

Merged

eyalgu reviewed Mar 30, 2020

View reviewed changes

digitalbuddha reviewed Apr 7, 2020

View reviewed changes

update api file

2a7e462

yigit commented Apr 18, 2020

View reviewed changes

add tests for the new Fetcher response API, add some docs

795cf06

digitalbuddha reviewed Apr 19, 2020

View reviewed changes

digitalbuddha reviewed Apr 20, 2020

View reviewed changes

eyalgu added a commit that referenced this pull request Apr 21, 2020

Introduce Fetcher from #139

81548bb

eyalgu suggested changes Apr 22, 2020

View reviewed changes

eyalgu mentioned this pull request Apr 29, 2020

When should we have the sample app point to released artifacts? #161

Closed

eyalgu linked an issue Apr 29, 2020 that may be closed by this pull request

When should we have the sample app point to released artifacts? #161

Closed

eyalgu removed a link to an issue Apr 29, 2020

When should we have the sample app point to released artifacts? #161

Closed

yigit closed this May 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DO NOT MERGE - WIP Make fetcher a class to avoid the API issues #139

DO NOT MERGE - WIP Make fetcher a class to avoid the API issues #139

Uh oh!

yigit commented Mar 23, 2020

Uh oh!

eyalgu Mar 30, 2020

Uh oh!

digitalbuddha Apr 7, 2020

Uh oh!

yigit Apr 11, 2020

Uh oh!

digitalbuddha left a comment

Uh oh!

yigit left a comment

Uh oh!

yigit Apr 18, 2020

Uh oh!

yigit Apr 18, 2020

Uh oh!

digitalbuddha Apr 19, 2020

Uh oh!

digitalbuddha Apr 20, 2020

Uh oh!

eyalgu left a comment

Uh oh!

eyalgu commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DO NOT MERGE - WIP Make fetcher a class to avoid the API issues #139

DO NOT MERGE - WIP Make fetcher a class to avoid the API issues #139

Uh oh!

Conversation

yigit commented Mar 23, 2020

Uh oh!

eyalgu Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

digitalbuddha Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

yigit Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

digitalbuddha left a comment

Choose a reason for hiding this comment

Uh oh!

yigit left a comment

Choose a reason for hiding this comment

Uh oh!

yigit Apr 18, 2020

Choose a reason for hiding this comment

Uh oh!

yigit Apr 18, 2020

Choose a reason for hiding this comment

Uh oh!

digitalbuddha Apr 19, 2020

Choose a reason for hiding this comment

Uh oh!

digitalbuddha Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

eyalgu left a comment

Choose a reason for hiding this comment

Uh oh!

eyalgu commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants