-
Notifications
You must be signed in to change notification settings - Fork 199
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
Adding cancellable interface to RemoteSyncEngine components #323
Conversation
...AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine+IncomingEventReconciliationQueueEvent.swift
Show resolved
Hide resolved
self.remoteSyncTopicPublisher.send(.performedInitialSync) | ||
self.stateMachine.notify(action: .performedInitialSync) | ||
} | ||
semaphore.signal() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in the TODO on L213 above, I assume we'll be removing the semaphore once we complete the cancelation wiring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe what you are proposing is reusing the recieve/recieveCompletion:
strategy we are using between the remoteSyncEngine and the Subscriptions with the initialSync code. I hadn't thought about it but yes, we could do something similar down the line.
...e/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/AWSIncomingEventReconciliationQueue.swift
Show resolved
Hide resolved
|
||
import Foundation | ||
|
||
//Note: There is already a Cancellable in Core/Support/Cancellable.swift, which does not seem to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than re-implement, import the core cancellable with import Amplify
where needed. Otherwise we'll have Combine.Cancellable, Amplify.Cancellable, and AWSDataStoreCategoryPlugin.Cancellable all potentially conflicting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried importing cancellable with import Amplify, and tried extending classes with Amplify.Cancellable, but wasn't able to fix this. I opened an issue here to take a look at cleaning this up:
#328
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, by looking at the type declaration I don't see anything that could block it from being used outside of core when importing Amplify
. Is it an Xcode cache issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two comments, but since we synced up about this PR already, I'll approve. If you believe the comments make sense and further action is needed, feel free to follow up with another PR.
asyncEvents.cancel() | ||
mapper = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- should
mapper
be cancelable as well? - does
asyncEvents
require cleanup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and yes. These two items are fixed here: :)
https://github.com/aws-amplify/amplify-ios/pull/326/files
|
||
import Foundation | ||
|
||
//Note: There is already a Cancellable in Core/Support/Cancellable.swift, which does not seem to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, by looking at the type declaration I don't see anything that could block it from being used outside of core when importing Amplify
. Is it an Xcode cache issue?
2a49d3a
to
782de9c
Compare
bb1b536
to
d7bfd59
Compare
In this PR, we are adding a cancellable interface, but still not calling cancel -- that will be done in the next PR
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.