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

forkJoin operator doesn't accept an array of observables #594

Closed
sumigoma opened this issue Oct 24, 2015 · 6 comments · Fixed by #759
Closed

forkJoin operator doesn't accept an array of observables #594

sumigoma opened this issue Oct 24, 2015 · 6 comments · Fixed by #759

Comments

@sumigoma
Copy link

ForkJoin doesn't seem to maintain the behavior from Rx4 of supporting both .forkJoin(a,b,c) and .forkJoin([a,b,c]). If you try to do the latter (an array of observables) it gives the exception

TypeError: observables[i].subscribe is not a function
    at ForkJoinObservable.execute.ForkJoinObservable._subscribe (ForkJoinObservable.ts:19)
@endash
Copy link

endash commented Oct 24, 2015

I'm just gonna comment here because my issue is virtually identical. This is also an issue with combineLatest. In Rx4 you could pass an array of observables followed by a projection function. In Rx5 you can only use the arguments list. It is very common to have an array of observables rather than a set number of variables of type Observable.

@trxcllnt
Copy link
Member

trxcllnt commented Nov 7, 2015

@Blesh I also think this is a concern. Unfortunately, TypeScript isn't exactly helpful in this regard. The only solution is to splat the Array when you call forkJoin/combineLatest, which you can only do in es6/ typescript.

I would be ok with a call to "flatten" arguments into a flat list of Observables, no matter how deeply-nested.

@kwonoj
Copy link
Member

kwonoj commented Nov 9, 2015

I was thinking about this bit, as @trxcllnt mentioned it requires some additional code to make it work with typescript. Trying to create some changes to trigger perf comparison to see if it's not hurting.

@benlesh
Copy link
Member

benlesh commented Nov 9, 2015

I'm sort of indifferent about this one.

We can try adding the checks and see how it affects the performance. If it's a hit, then I say we don't do it, and people can just use .apply like they would on any other function in JavaScript.

@kwonoj
Copy link
Member

kwonoj commented Nov 9, 2015

how it affects the performance

: That's I'd like to verify :) if changes does not hurt performance, supporting this would be good. I should come up with numbers.

@kwonoj
Copy link
Member

kwonoj commented Nov 9, 2015

Initial attempt blocked due to same reason as #660, property based stub does not allow overload. Trying some other approaches.

kwonoj added a commit to kwonoj/rxjs that referenced this issue Nov 12, 2015
kwonoj added a commit to kwonoj/rxjs that referenced this issue Nov 13, 2015
kwonoj added a commit to kwonoj/rxjs that referenced this issue Nov 13, 2015
kwonoj added a commit to kwonoj/rxjs that referenced this issue Nov 13, 2015
kwonoj added a commit that referenced this issue Nov 18, 2015
kwonoj added a commit to kwonoj/rxjs that referenced this issue Nov 20, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants