feat(DataStore): Combine support for all APIs #524
Conversation
AmplifyCombineSupport/AmplifyCombineSupport.xcodeproj/project.pbxproj
Outdated
Show resolved
Hide resolved
| if case .failure = completion { | ||
| receivedError.fulfill() |
There was a problem hiding this comment.
is there a way to know that it was when saving the PhoneCall that failed?
There was a problem hiding this comment.
Not unless the failing operation enhances the error in some way that makes it obvious. In that way, this pattern is similar to a do/catch block, or a Promise chain in the async world--error handling is consolidated in one place.
There was a problem hiding this comment.
This looks great! Very happy to see this coming together. I have a couple of final notes:
Milestone:
I think this is a great add to target for 1.1.0, so my question comes down to how should we manage milestones in parallel. Currently with our branching strategy, once this is merged to master it means it will be in the next release. How can we manage parallel minor/path releases? I have created an item in our backlog to discuss our Git branching model, I would like to hear your thoughts on it, I think this PR is a precedent to that discussion.
Boilerplate:
It seems they will be a lot of boilerplate involved and effort maintaining as we change/add APIs. I would love to discuss the possibility of automating extensions like this, maybe using something like https://github.com/krzysztofzablocki/Sourcery to help us automate the boilerplate maintenance?
AmplifyCombineSupport.podspec
Outdated
| s.homepage = 'https://aws.amazon.com/amplify/' | ||
| s.license = 'Apache License, Version 2.0' | ||
| s.author = { 'Amazon Web Services' => 'amazonwebservices' } | ||
| s.source = { :git => 'https://github.com/aws-amplify/amplify-ios.git', :tag => s.version} |
There was a problem hiding this comment.
our tags are prefixed with v, I fixed this for the other podspecs in this PR: #538
There was a problem hiding this comment.
Thanks for this callout, will fix.
|
|
||
| public typealias DataStorePublisher<Output> = AnyPublisher<Output, DataStoreError> | ||
|
|
||
| public extension DataStoreBaseBehavior { |
There was a problem hiding this comment.
question: as far as developer experience goes, is there a way to apply this extension when customers import Combine in their source file (as opposed to import AmplifyCombineSupport)?
I realized that PromiseKit follows that pattern, but it seems they use some macro magic and I'm not sure what's the support for that: https://github.com/PromiseKit/CoreLocation/blob/c4ea4dba6e0516986b3cb1d93c86f6991ce46861/Sources/CLGeocoder%2BPromise.swift#L1-L4
There was a problem hiding this comment.
Will investigate to see if it's possible to automatically import this as part of import Amplify; import Combine as long as the support library is installed. Considerations:
- Can the solution apply to non-CocoaPods installations (e.g., SPM, Carthage, framework)?
- Does the solution feel not-too-surprising? E.g., I would rather be explicit and require an
import AmplifyCombineSupportuntil we officially roll it into Amplify Core in a couple of years, than make customers wonder why they have toimport Combineto use the Combine-flavored APIs, even though they’re using any core Combine APIs in that file. Realistically this might not be a problem, since you'll often have to store a reference to anAnyCancellableorPublisherof some kind, which requires a Combine import.
My initial instinct is to merge it in when we're ready for it, where "ready" is a combination of: "the code is ready", and "we're ready to take it." Adding additional processes (long-lived, release-targeted branches) feels like the wrong way to go, especially as we try to move releases to be more automated, low-touch events. To that end, I'll cut a I'm happy to entertain other ideas, but that seems like the simplest, least-overhead way to go for now.
I don't object to bringing in dependencies to help with builds and tests--see SwiftFormat, SwiftLint, CwlWithLove*, etc. Sourcery seems like a nice win if we can get it to take care of the boilerplate for us. We'd have to experiment to make sure it handles transformations as we want it to, especially in the progress-reporting APIs, which we haven't yet addressed with the Combine Support. |
Adds a new AmplifyCombineSupport pod that provides support for Combine flavors of Amplify APIs. Currently only supports DataStore.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.