-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Update .cancellable implementation to fix #946
#949
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
Update .cancellable implementation to fix #946
#949
Conversation
| environment: environment | ||
| ) | ||
|
|
||
| store.send(.recordButtonTapped) |
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.
.recordButtonTapped ends up returning a merged effect
swift-composable-architecture/Examples/VoiceMemos/VoiceMemos/VoiceMemos.swift
Lines 70 to 76 in 313dd21
| return .merge( | |
| environment.audioRecorder.startRecording(url) | |
| .catchToEffect(VoiceMemosAction.audioRecorder), | |
| Effect.timer(id: TimerId(), every: 1, tolerance: .zero, on: environment.mainRunLoop) | |
| .map { _ in .currentRecordingTimerUpdated } | |
| ) |
using
RunLoop.immediate results in a failed test because .currentRecordingTimerUpdated now also gets received once before .audioRecorder(.failure(.couldntActivateAudioSession)) is received. I'm unsure if this is an undesired result of the change.
Adding the following test to TimerTests.swift demonstrates the difference in behaviour.
func testTimerImmediate() {
let scheduler = DispatchQueue.immediate
var count = 0
Effect.timer(id: 1, every: .seconds(1), on: scheduler)
.sink { _ in count += 1 }
.store(in: &self.cancellables)
XCTAssertNoDifference(count, 1) // This PR
// XCTAssertNoDifference(count, 0) // Current implementation
}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.
For the current implementation, the timer is triggered as soon as the PassThroughSubject subscribes to it, essentially dropping the value before the effect is subscribed to downstream.
swift-composable-architecture/Sources/ComposableArchitecture/Effects/Cancellation.swift
Lines 36 to 37 in 313dd21
| let subject = PassthroughSubject<Output, Failure>() | |
| let cancellable = self.subscribe(subject) |
So the timer auto connects and outputs a value at line 37 which is lost before returning the effect.
In this PR the timer auto connects at .sink { _ in count += 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.
Interesting find! I think overall this PR is an improvement in the expectation that at least 1 thing emits from a timer running on an immediate scheduler, even if it is a breaking change.
stephencelis
left a comment
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.
Great work @iampatbrown! Thanks for digging into the problem.
Possible fix for #946
Main issue with
PassthroughSubjectis that it could only forward 1 value from an upstreamPassthroughSubject, ie. nested cancellable effects. This should fix that by subscribing directly to the upstream publisher.