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

Rx.Observable.merge should return same type regardless of input #2274

Closed
ORESoftware opened this issue Jan 12, 2017 · 5 comments
Closed

Rx.Observable.merge should return same type regardless of input #2274

ORESoftware opened this issue Jan 12, 2017 · 5 comments

Comments

@ORESoftware
Copy link

ORESoftware commented Jan 12, 2017

btroncone/learn-rxjs#26 (comment)

Mr. btroncone and I are both curious and concerned about the API for Rx.Observable.merge -

it seems to return different types based off the input, which is at the very least confusing.

You can see the difference when comparing Rx.Observable.of with Rx.Observable.merge, see code below.

RxJS version:

5.0.2

Code to reproduce:

const Rx = require('rxjs');

var example = Rx.Observable.of(
    Rx.Observable.timer(1),
    Rx.Observable.timer(1),
    Rx.Observable.timer(1)
);

console.log(example.constructor.name);

var example = Rx.Observable.of([
        Rx.Observable.timer(1),
        Rx.Observable.timer(1),
        Rx.Observable.timer(1)
    ]);

console.log(example.constructor.name);


var example = Rx.Observable.merge(
    Rx.Observable.timer(1),
    Rx.Observable.timer(1),
    Rx.Observable.timer(1)
);

console.log(example.constructor.name);

var example = Rx.Observable.merge([
    Rx.Observable.timer(1),
    Rx.Observable.timer(1),
    Rx.Observable.timer(1)
]);

console.log(example.constructor.name);

The output is:

ArrayObservable
ScalarObservable
Observable
Array

Expected behavior:

ArrayObservable
ScalarObservable
Observable
SomeObservable  // <<<

Actual behavior:

ArrayObservable
ScalarObservable
Observable
Array  // <<< an array?

Additional information:

Maybe there is a good reason for this, but it seem bad on the face of things

@paulpdaniels
Copy link
Contributor

merge does not accept an Array. See the declarations.

If you pass it a single array argument you would hit this conditional, which would just return the Array back casted to an Observable.

If you want to use it with an array you need to destructure it, the following should work:

var example = Rx.Observable.merge(...[
    Rx.Observable.timer(1),
    Rx.Observable.timer(1),
    Rx.Observable.timer(1)
]);

@ORESoftware
Copy link
Author

ok so here is the thing:

if it doesn't currently accept an array (which seems clear) - it should accept an array, because it obviously should given the arguments it accepts; but if the API really really can't accept an array, then it should throw an error IMO.

if it does accept an array, then please make the return value consistent, in this case, an Observable

I like TypeScript and am using it to interface with RxJS in some case, but in other cases I am interfacing with RxJS with plain JS and nothing is really stopping me from passing an array to the merge method etc.

It would help if:

(a) it allowed for arrays, (which it really should)
or
(b) throw an error if an array is passed

my 2 cents

@kwonoj
Copy link
Member

kwonoj commented Jan 18, 2017

I am considering this issue as same as #2294 and #2144, about runtime validation and soundness, linking each.

@kwonoj
Copy link
Member

kwonoj commented Apr 16, 2017

I'll use #2153 as umbrella issue to discuss runtime validations.

@kwonoj kwonoj closed this as completed Apr 16, 2017
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 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

No branches or pull requests

3 participants