Skip to content
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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

VAndrJ
Copy link

@VAndrJ VAndrJ commented May 14, 2024

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

MRE
import SwiftUI
import RxSwift
import RxCocoa

struct ContentView: View {
    private let valuesObs = PublishRelay<Int>()
    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
Copy link
Collaborator

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

@freak4pc
Copy link
Member

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
Copy link
Collaborator

I know this has been an issue since at least 5.0.0 (see for eg: #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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants