ReactiveX / RxSwift

Reactive Programming in Swift
MIT License
24.32k stars 4.17k forks source link

Fix `.throttle` with `.seconds()` rounding issue. #2600

Closed VAndrJ closed 1 month ago

VAndrJ commented 4 months ago

When using .seconds(1), 2 elements are emitted per second. MRE with problem reproduction:

MRE ```swift import SwiftUI import RxSwift import RxCocoa struct ContentView: View { private let valuesObs = PublishRelay() private let bag = DisposeBag() init() { valuesObs .throttle(.milliseconds(1000), latest: true, scheduler: MainScheduler.instance) .subscribe(onNext: { print(".milliseconds(1000) throttle date: \(formatter.string(from: Date())), value: \($0)") }) .disposed(by: bag) valuesObs .throttle(.seconds(1), latest: true, scheduler: MainScheduler.instance) .subscribe(onNext: { print(".seconds(1) throttle date: \(formatter.string(from: Date())), value: \($0)") }) .disposed(by: bag) } var body: some View { Image(systemName: "globe") .task { for i in 0...999 { valuesObs.accept(i) try? await Task.sleep(for: .milliseconds(100)) } } } } let formatter: DateFormatter = { let formatter = DateFormatter() formatter.dateFormat = "mm:ss.SSS" return formatter }() ```

Result of MRE:


.milliseconds(1000) throttle date: 24:50.981, value: 0
.seconds(1) throttle date: 24:50.981, value: 0
.seconds(1) throttle date: 24:51.508, value: 5
.milliseconds(1000) throttle date: 24:51.983, value: 9
.seconds(1) throttle date: 24:52.037, value: 10
.seconds(1) throttle date: 24:52.566, value: 15
.milliseconds(1000) throttle date: 24:52.984, value: 19
.seconds(1) throttle date: 24:53.084, value: 20
.seconds(1) throttle date: 24:53.604, value: 25
...

As we can see, due to rounding, 2 elements are emitted per second, but only one should.

danielt1263 commented 3 months ago

I like this change. It's something that has always bugged me. It should probably wait for the major version bump.

freak4pc commented 3 months ago

Seems like a good fix! I'm wondering if we can somehow pull it into the current release by doing something like thorttle(..., rounding: FloatingPointRoundingRule = . toNearestOrAwayFromZero) and being able to override it to up.

(Or think of a different "behavior" argument). This will allow to be backward compatible during RxSwift 6.x and then make this the default behavior in RxSwift 7.x.

Thoughts?

danielt1263 commented 3 months ago

I know this has been an issue since at least 5.0.0 (see for eg: https://github.com/ReactiveX/RxSwift/issues/2098) And I've always used milliseconds in production code because of it. So I know none of my apps would be affected by the fix.

I like the idea of adding the parameter so we can put this fix in sooner...