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

Modified behavior on empty inputs for forkJoin, combineLatest and zip #5922

Closed
wants to merge 6 commits into from
Closed

Modified behavior on empty inputs for forkJoin, combineLatest and zip #5922

wants to merge 6 commits into from

Conversation

rraziel
Copy link
Contributor

@rraziel rraziel commented Dec 8, 2020

Description:

The forkJoin, combineLatest and zip functions have been modified so that:

combineLatest();   // Observable<never>: behavior kept for compatibility as the non-array version is deprecated
combineLatest([]); // Observable<[]>: now emits an empty array before completing
combineLatest({}); // Observable<{}>: now emits an empty object before completing

forkJoin();   // Observable<never>: behavior kept for compatibility as the non-array version is deprecated
forkJoin([]); // Observable<[]>: now emits an empty array before completing
forkJoin({}); // Observable<{}>, now emits an empty object before completing

zip();     // Observable<never>, behavior kept for compatibility as the non-array version is now deprecated
zip(a,b)   // /!\ has been marked as deprecated in this PR /!\
zip([]);   // Observable<[]>: now emits an empty array before completing
zip({a,b}) // Observable<{a: A, b: B}>: new behavior to align with the other ones
           // /!\ new behavior to align with the other ones /!\
zip({});   // Observable<{}>: emits an empty object then completes

// side effects
zipAll() // now emits [] for empty observables

Related issue (if exists):

Regarding the signature (#5066), in particular needs N-args signature for observable inputs:
All 3 functions seem to have the correct <A extends readonly unknown[]>(sources: [...ObservableInputTuple<A>]): Observable<A>

Closes #5209

@rraziel
Copy link
Contributor Author

rraziel commented Dec 12, 2020

Note: if the deprecated signatures can be removed because of the major version bump (except for the ones I just added on zip I suppose), I can update the PR; let me know

@rraziel
Copy link
Contributor Author

rraziel commented Jan 22, 2021

@benlesh The fix in 7.0.0-beta.10 contradicts this PR and #5209 's comments, should this PR be dropped?

@benlesh
Copy link
Member

benlesh commented Jan 26, 2021

I really appreciate the effort that went into here, but I don't think we can accept this PR.

With these creation functions, the supplied object or array represents a dictionary or list of observable sources, respectively. If those are empty ({} or []), then there are no observables to subscribe to, and therefor, nothing to emit values to emit as an array of values or a dictionary of values... So the behavior should be that it emits an EMPTY which is Observable<never>.

I hope that makes sense.

Thanks again for your work!

@benlesh benlesh closed this Jan 26, 2021
@rraziel
Copy link
Contributor Author

rraziel commented Jan 26, 2021

@benlesh No worries, I never got an answer on the comments from #5209 so it's not like anyone told me "go for it" (would have been nice to get a "don't go for it" though 😅)
Should I still create a PR for the zip({}) code that was also in there (same as forkJoin({}) and combineLatest({}))?

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.

forkJoin should emit empty array or object given empty array or object as input
2 participants