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 Infallible's flatMap family of operators that can return an Infallible that is not, in fact, infallible #2536

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

Conversation

nikolaykasyanov
Copy link
Contributor

@nikolaykasyanov nikolaykasyanov commented Jul 18, 2023

I believe it's a big hole in the Infallible contract because this will a) compile b) return an Infallible that will fail silently, without calling any subscription closures (well maybe it will call onDispose[d], though, but that's beside the point):

_= someInfallible.flatMap { Observable.error(anError) }
    subscribe(onCompleted: { print("it's not gonna call me") }) 

There's two ways to fix this:

  1. Have overloads for both variants, i.e.
    1. a flatMap that expects its closure to only return Infallible streams, and that itself returns Infallible.
    2. a flatMap that expects its closure to return any ObservableConvertibleType and itself returns Observable.
  2. Only have the first of the above.

While 1. is better for backwards compatibility, it complicates overload resolution (Infallible is ObservableConvertibleType), although it seems to be working in the test code I've written.

As soon as the proposed change is agreed on, I'll add tests for the first and latest variants.

@nikolaykasyanov nikolaykasyanov changed the title Fix Infallible's flatMap family of operators that can return an Infallible that fails silently Fix Infallible's flatMap family of operators that can return an Infallible that is not, in fact, infallible Jul 18, 2023
Projects each element of an infallible sequence into a new sequence of infallible sequences and then
transforms the infallible sequence of infallible sequences into an infallible sequence producing values only from the most recent infallible sequence.

It is a combination of `map` + `switchLatest` operator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that switchLatest doesn't exist for Infallible, should this line be kept or not?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. It should be merged and released 🙏

Projects each element of an infallible sequence into a new sequence of infallible sequences and then
transforms the infallible sequence of infallible sequences into an infallible sequence producing values only from the most recent infallible sequence.

It is a combination of `map` + `switchLatest` operator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be removed to avoid misunderstandings

@freak4pc
Copy link
Member

I think the fix could make sense indeed, but unfortunately it's a breaking API change so we can't easily merge it right now. It will have to wait for a later major version, but I'm keeping it open to not forget. Thanks!

@freak4pc freak4pc force-pushed the main branch 2 times, most recently from 1198683 to 5949cbd Compare April 21, 2024 07:09
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.

4 participants