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

Filter out errors from CompletableAction.completions #236

Conversation

sdanny
Copy link
Contributor

@sdanny sdanny commented Jul 23, 2021

Problem statement
CompletableAction is usually used in some repeatable manner. Each action can finish with an error especially if there's a network request or a validation process inside the workFactory. So far completions emits these errors which in turn disposes existing subscriptions to this observable.

Proposed solution
Catch errors in completions.
#trivial

@ashfurrow
Copy link
Member

Seems reasonable, but it would be a breaking API change. I'll request some reviews from others to gather feedback 👍

@mosamer
Copy link
Contributor

mosamer commented Jul 27, 2021

Seems reasonable, but it would be a breaking API change. I'll request some reviews from others to gather feedback 👍

LGTM as well. The change is behavioural as far as I can see, API itself is intact. AFAICT, it doesn't contradict with neither how it is named completions nor the existing documentation.

@sdanny We may need however to detail documentation a bit and add entry to CHANGELOG

@ashfurrow
Copy link
Member

@mosamer thank you 🙌 I agree the shape of the API itself doesn't change, but the semantics do in a way that warrants a major version bump. The current version is 4.3.0, so the next version with this change should be 5.0.0.

@mosamer
Copy link
Contributor

mosamer commented Jul 28, 2021

@mosamer thank you 🙌 I agree the shape of the API itself doesn't change, but the semantics do in a way that warrants a major version bump. The current version is 4.3.0, so the next version with this change should be 5.0.0.

@ashfurrow IMHO this is more of a bug fix, comparing it to elements (which emits next events values instead of complete event) this stream should not dispose upon errors. That said, I agree bumping up to 5.0.0 may be a safer choice so I am fine with it too.

Copy link
Member

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

Cool, it's been a week and we've gotten no more feedback, so let's merge 👍 I'll get this deployed ASAP and follow up.

@mosamer mosamer merged commit ca75632 into RxSwiftCommunity:master Aug 5, 2021
@rxswiftcommunity
Copy link

Thanks a lot for contributing @sdanny! I've invited you to join the
RxSwiftCommunity GitHub organization – no pressure to accept! If you'd like
more information on what this means, check out our contributor guidelines
and feel free to reach out with any questions.

Generated by 🚫 dangerJS

@ashfurrow
Copy link
Member

Cool! This is released as version 5.0.0. I ran into this error from CocoaPods during pod trunk push but got around it by adding --skip-import-validation. Thanks again!

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.

3 participants