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.

Sign up for GitHub

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

Jump to bottom

Joining empty list of producers fix #774

Merged
merged 8 commits into from
May 22, 2020
Merged

Joining empty list of producers fix #774

merged 8 commits into from
May 22, 2020

Conversation

rocketnik
Copy link
Contributor

Checklist

✅ Updated CHANGELOG.md.

Nikolas Mayr added 2 commits February 13, 2020 15:31
joining an empty list of producers returns a single event of the coll…
0e08a57 
…ected results, which is the empty array - this becomes relevant, when the list of producers is calculated and you are merging the resulting joined signal with some other signal. the end result will stall, if no event is sent from the joined producers.
updated changelog
7c18810
@rocketnik rocketnik changed the title Nm/joining empty list of producers fix Joining empty list of producers fix Feb 13, 2020
@andersio
Copy link
Member

andersio commented Feb 18, 2020 •
edited
Loading

Both combineLatest and zip have been designed to consistently emit only array of N elements, given a sequence of N producers, and are documented to return an empty producer if the sequence of producer is empty. So to avoid breaking existing clients silently, we cannot change the default behavior here. I do see the ergonomic value here though.

The best option, IMO, would be to make it an opt-in option, e.g. noUpstreamSentinel: [S.Iterator.Element.Value]? on combineLatest and zip that accepts a Sequence.

@rocketnik
Copy link
Contributor Author

@andersio please check again.

I agree with not breaking old code and went with your idea.

For private functions, I made this parameter mandatory, while for public ones I gave it a default value of nil resulting in the exact same behavior as before.

Please notice the implementation of func all and func and, for which I have provided a sentinel ([]). It looked like the old implementation was expecting an empty list to be returned already, but actually, this could not have happened.

Merge branch 'master' of github.com:ReactiveCocoa/ReactiveSwift into …
dbf76a3 
…nm/joining_empty_list_of_producers_fix

# Conflicts:
#	CHANGELOG.md
@rocketnik
Copy link
Contributor Author

Resolved conflict. @andersio Please recheck, when you find the time.

@rocketnik
Copy link
Contributor Author

Any update on this?

1 similar comment
@rocketnik
Copy link
Contributor Author

Any update on this?

andersio
andersio approved these changes May 22, 2020
View reviewed changes
Copy link
Member

@andersio andersio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for late reply. The apporach LGTM! I will tweak the doucmentation & indentation afterwards. :shipit:

@andersio andersio merged commit 6f308d6 into ReactiveCocoa:master May 22, 2020
@andersio andersio mentioned this pull request May 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andersio andersio andersio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rocketnik @andersio