feat: Add Combine support #667
Conversation
palpatim
force-pushed
the
palpatim/combine-support-take-2
branch
from
1c2f420 to
4925f68
Compare
| // No-listener versions of the public APIs, to clean call sites that use Combine | ||
| // publishers to get results |
There was a problem hiding this comment.
I am a little bit confused by this strategy. I understand why the listener is required, but making it optional only on iOS 13 is no guarantee that the developer will consume the result through the Combine publisher, is that fair assumption?
Can we provide these extensions passing nil by default regardless of the iOS version? I believe this would be beneficial for other 3rd party integrations as well, such as PromiseKit. Otherwise we, or the community, will have to write the same extensions for PromiseKit support.
What are your thoughts on this?
There was a problem hiding this comment.
ps: one thing that just came to my mind is, JS Promises emits warnings when no listeners are attached to a promise, that could be an interesting DX optimization for nil listeners that weren't resolved anywhere (either by the developer, or by a Combine publisher, or by a PromiseKit Promise)... Just food for thought! :)
There was a problem hiding this comment.
I am a little bit confused by this strategy. I understand why the listener is required, but making it optional only on iOS 13 is no guarantee that the developer will consume the result through the Combine publisher, is that fair assumption?
That is correct.
Can we provide these extensions passing
nilby default regardless of the iOS version? I believe this would be beneficial for other 3rd party integrations as well, such as PromiseKit. Otherwise we, or the community, will have to write the same extensions for PromiseKit support.What are your thoughts on this?
We definitely could. It would mean that customers who don't support iOS 13 would be able to execute operations with no listener, just like Combine-flavored APIs. It would simplify the API surface and bring parity to the party, too, so that's a plus.
The main question would be whether we feel comfortable with giving customers the ability to invoke without a listener, but I'm thinking that might be OK. We've made a pretty strong commitment to the paradigm that "the work being performed by an operation" is separate from "the reporting of the results of the operation", by separating cancellation of the work from cancellation of the subscription.
I think I'm in favor of this and will update.
There was a problem hiding this comment.
Force-push includes a rebase/squash, and giving default values to nil-listener APIs for all platforms, not just iOS 13.
palpatim
force-pushed
the
palpatim/combine-support-take-2
branch
2 times, most recently
from
dcc166f to
8e8c0b7
Compare
- feat: Amplify Category APIs that accept optional listeners now default them to `nil`, simplifying the call site - feat: HubChannel is now Hashable - feat: Add 'underlying error' constructor to AmplifyError - chore: Fix OperationTestBase and reenable previously disabled tests - chore: Change GraphQLOperation and GraphQLSubscriptionOperation to subclasses rather than typealiases
palpatim
force-pushed
the
palpatim/combine-support-take-2
branch
from
8e8c0b7 to
e4b12f8
Compare
This PR adds Combine support to Amplify asynchronous APIs that return results. See
README-combine-support.mdfor details.Changes:
AmplifyErrorprotocol now requires conforming types to supply an initializer that takes a description, recovery suggestion, and underlying error. Most existing AmplifyError types will map this to anunknowncaseGraphQLOperationis now a concrete subclass ofAmplifyOperationinstead of a typealiasGraphQLSubscriptionOperationis now a concrete subclass ofAmplifyInProcessReportingOperationinstead of a typealiasSuccessandFailuretypealiases toAmplifyOperation, to make generics easier to work withInProcesstypealias toAmplifyInProcessReportingOperation, to make generics easier to work withHubChannelnow conforms toHashableAtomicValue.withmethod to allow for manipulations of a value inside a blockAWSAPICategoryPluginto make a dependencies easier to inject at configurationBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.