-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Live stream subscription state refresh #77
Conversation
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.
just a few small comments. i didn't go too deeply into it, but it all makes sense to me at a high level and i bet the tests cover it well!
internal func subscribeTo(eventId: Int, uid: Int, isSubscribed: Bool) | ||
-> SignalProducer<Bool, LiveApiError> { | ||
internal func subscribeTo(eventId: Int, uid: Int, isSubscribed: Bool) -> SignalProducer< | ||
LiveStreamSubscribeEnvelope, LiveApiError> { |
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.
let's keep the type SignalProducer<LiveStreamSubscribeEnvelope, LiveApiError>
all on one line
@@ -31,5 +31,6 @@ public protocol LiveStreamServiceProtocol { | |||
func signInAnonymously(completion: @escaping (String) -> Void) | |||
|
|||
/// Subscribes/unsubscribes a user to an event. | |||
func subscribeTo(eventId: Int, uid: Int, isSubscribed: Bool) -> SignalProducer<Bool, LiveApiError> | |||
func subscribeTo(eventId: Int, uid: Int, isSubscribed: Bool) -> SignalProducer<LiveStreamSubscribeEnvelope, | |||
LiveApiError> |
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.
same here
@@ -160,7 +160,8 @@ public struct LiveStreamService: LiveStreamServiceProtocol { | |||
} | |||
} | |||
|
|||
public func subscribeTo(eventId: Int, uid: Int, isSubscribed: Bool) -> SignalProducer<Bool, LiveApiError> { | |||
public func subscribeTo(eventId: Int, uid: Int, isSubscribed: Bool) -> SignalProducer< | |||
LiveStreamSubscribeEnvelope, LiveApiError> { |
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.
and here!
XCTAssertEqual(true, eventsEnvelope.value?.success) | ||
} | ||
|
||
func testParseJson_Failure() { |
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.
nice thanks for covering all the cases
public enum lens { | ||
public static let success = Lens<LiveStreamSubscribeEnvelope, Bool>( | ||
view: { $0.success }, | ||
set: { var new = $1; new.success = $0; return new } |
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.
yahhhh big fan of these lenses now
.fetchEvent(eventId: event.id, uid: AppEnvironment.current.currentUser?.id) | ||
.ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler) | ||
.materialize() | ||
}.take(first: 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.
i don't think the take(first: 1)
is needed since this can only emit a single time
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.
you can also prefix(value: event)
in the switchMap
, and then the signal below just becomes let event = updatedEventFetch.values()
When I merged #63 some behaviour was changed regarding how the
LiveStreamEvent
is configured on theLiveStreamEventDetailsViewModel
which emits the subscription state for that live stream. Because the change meant that the view model is configured with theLiveStreamEvent
that is fetched on the project page, it didn't contain any subscription info. This also wasn't immediately apparent because we didn't have great error handling around the subscribe call. In the past we would receive a200
but thesuccess
value in the JSON wasfalse
. Now we check for that.This fixes all of that:
LiveStreamSubscribeEnvelope
so that the response from the subscribe call is a real thing.LiveStreamEvent
on load to get the latest subscription status.