-
Notifications
You must be signed in to change notification settings - Fork 1
Add StateLayerEventPublisher for centralised change handling #15
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
Add StateLayerEventPublisher for centralised change handling #15
Conversation
Use it in Activity and ActivityState Cover Activity type with tests
if flag { | ||
let response = try await apiClient.pinActivity(feedGroupId: feed.group, feedId: feed.id, activityId: activityId) | ||
return response.activity.toModel() | ||
} else { | ||
let response = try await apiClient.unpinActivity(feedGroupId: feed.group, feedId: feed.id, activityId: activityId) | ||
return response.activity.toModel() | ||
} |
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.
Found a bug while writing tests, unpin was never called
Generated by 🚫 Danger |
Public Interface public final class Activity: Sendable
- @discardableResult public func addCommentReaction(commentId: String,request: AddCommentReactionRequest)async throws -> FeedsReactionData
+ @discardableResult public func addReaction(request: AddReactionRequest)async throws -> FeedsReactionData
- @discardableResult public func deleteCommentReaction(commentId: String,type: String)async throws -> FeedsReactionData
+ @discardableResult public func deleteReaction(type: String)async throws -> FeedsReactionData
- public func pin()async throws
+ @discardableResult public func addCommentReaction(commentId: String,request: AddCommentReactionRequest)async throws -> FeedsReactionData
- public func unpin()async throws
+ @discardableResult public func deleteCommentReaction(commentId: String,type: String)async throws -> FeedsReactionData
- @discardableResult public func closePoll()async throws -> PollData
+ public func pin()async throws
- public func deletePoll(userId: String? = nil)async throws
+ public func unpin()async throws
- @discardableResult public func getPoll(userId: String? = nil)async throws -> PollData
+ @discardableResult public func closePoll()async throws -> PollData
- @discardableResult public func updatePollPartial(request: UpdatePollPartialRequest)async throws -> PollData
+ public func deletePoll(userId: String? = nil)async throws
- @discardableResult public func updatePoll(request: UpdatePollRequest)async throws -> PollData
+ @discardableResult public func getPoll(userId: String? = nil)async throws -> PollData
- @discardableResult public func createPollOption(request: CreatePollOptionRequest)async throws -> PollOptionData
+ @discardableResult public func updatePollPartial(request: UpdatePollPartialRequest)async throws -> PollData
- public func deletePollOption(optionId: String,userId: String? = nil)async throws
+ @discardableResult public func updatePoll(request: UpdatePollRequest)async throws -> PollData
- @discardableResult public func getPollOption(optionId: String,userId: String?)async throws -> PollOptionData
+ @discardableResult public func createPollOption(request: CreatePollOptionRequest)async throws -> PollOptionData
- @discardableResult public func updatePollOption(request: UpdatePollOptionRequest)async throws -> PollOptionData
+ public func deletePollOption(optionId: String,userId: String? = nil)async throws
- @discardableResult public func castPollVote(request: CastPollVoteRequest)async throws -> PollVoteData?
+ @discardableResult public func getPollOption(optionId: String,userId: String?)async throws -> PollOptionData
- @discardableResult public func deletePollVote(voteId: String,userId: String? = nil)async throws -> PollVoteData?
+ @discardableResult public func updatePollOption(request: UpdatePollOptionRequest)async throws -> PollOptionData
+ @discardableResult public func castPollVote(request: CastPollVoteRequest)async throws -> PollVoteData?
+ @discardableResult public func deletePollVote(voteId: String,userId: String? = nil)async throws -> PollVoteData? |
await withTaskGroup(of: Void.self) { group in | ||
for handler in handlers { | ||
group.addTask { | ||
await handler(event) | ||
} | ||
} | ||
} |
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.
Makes sure handler runs on a background thread which is good when handler wants to do complex filtering, aka local filter matching
SDK Size
|
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.
Looks good, left few comments. Also, I think there are still some bugs in the state layer.
private func subscribe(to publisher: StateLayerEventPublisher) { | ||
eventObserver = publisher.subscribe { [weak self, activityId, feed] event in | ||
switch event { | ||
case .activityUpdated(let activityData, let eventFeedId): |
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.
seems like we still have a bug with activity reactions - e.g. if the initial state is 1, you react, it becomes 3. Remove reaction, it becomes 1.
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.
Bookmarks also still don't work well (own issue, maybe backend?)
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 did a quick fix in this PR since the issue comes from the Feed type. Next PR is going to be about Feed, its event handling and adding tests.
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 was able to reproduce this myself as well and now it does not happen.
Sources/StreamFeeds/StateLayer/Common/StateLayerEventPublisher.swift
Outdated
Show resolved
Hide resolved
} | ||
|
||
extension StateLayerEventPublisher { | ||
final class ObservationToken: Sendable { |
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.
Maybe we call this Subscription? That's what we also use for video.
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.
Sounds good! Done
final class ObservationToken: Sendable { | ||
private let action: @Sendable () -> Void | ||
|
||
init(action: @escaping @Sendable () -> Void) { |
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.
Also action is a bit misleading here, maybe we should be more explicit here with the name?
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.
Renamed it to cancel
🔗 Issue Links
Related to: IOS-1090 & IOS-1122
🎯 Goal
Add centralised event handler for state-layer so that we can use it when receiving WS events and when calling changes manually after API calls.
📝 Summary
🛠 Implementation
Centralised handler allows having filtering in a single handler running on a background thread. Before the filtering was in the WS event observer only which causes issues when change handlers were called directly after API calls. Especially problematic with queries with filters where the change might not match the query anymore.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
🧪 Manual Testing Notes
☑️ Contributor Checklist
docs-content
repo