Skip to content

Incorrect types with switch? #2493

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

Closed
LOZORD opened this issue Mar 25, 2017 · 6 comments
Closed

Incorrect types with switch? #2493

LOZORD opened this issue Mar 25, 2017 · 6 comments
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@LOZORD
Copy link

LOZORD commented Mar 25, 2017

RxJS version:
5.2.0

Code to reproduce:

import { Observable } from 'rxjs';

function foo(): Observable<number[]> {
    return Observable.of([1, 2, 3, 4, 5]);
}

function bar(n: number): Observable<string> {
    return Observable.of(n.toString().repeat(n));
}

interface ValNum {
    val: number;
}

function baz(n: number): Observable<ValNum> {
    return Observable.of({ val: n * 10 });
}

interface Payload {
    myNumber: number;
    myString: string;
    myObject: ValNum;
}

function allTogetherNow(): Observable<Payload[]> {
    // Get an Observable of a list.
    const numsObs = foo();

    // Map every item in the Observable list to a new Observable based on
    // the returns of the other two functions.
    return numsObs.map(nums => {
        // For every number in the list:
        return nums.map(num => {
            // Return that number as an Observable.
            const numObs = Observable.of(num);
            // Then compute the other two Observables.
            const strObs = bar(num);
            const valObs = baz(num);

            // Zip all three together into the Payload object.
            return Observable.zip(
                numObs, strObs, valObs,
                (myNumber, myString, myObject): Payload =>
                    ({ myNumber, myString, myObject })
                // And then flatten to get the latest result.
            ).switch();
        });
    });
}

// We get back an Observable of Payloads, right?
allTogetherNow().subscribe(payloads => {
    console.log('Got payloads: ', payloads);
    const first = payloads[0];
    // It should be a Payload object?
    console.log(`It's not an Observable, right? `,
        !(first instanceof Observable));
});

Expected behavior:

Logs a list of Payload objects. Objects should not be instances of Observable.

Actual behavior:

Instead of Payload Objects in the subscribe function, I'm getting a list of Observables.

Here's the actual output I got:

Got payloads:  [ Observable {
    _isScalar: false,
    source: Observable { _isScalar: false, source: [Object], operator: [Object] },
    operator: SwitchOperator {} },
  Observable {
    _isScalar: false,
    source: Observable { _isScalar: false, source: [Object], operator: [Object] },
    operator: SwitchOperator {} },
  Observable {
    _isScalar: false,
    source: Observable { _isScalar: false, source: [Object], operator: [Object] },
    operator: SwitchOperator {} },
  Observable {
    _isScalar: false,
    source: Observable { _isScalar: false, source: [Object], operator: [Object] },
    operator: SwitchOperator {} },
  Observable {
    _isScalar: false,
    source: Observable { _isScalar: false, source: [Object], operator: [Object] },
    operator: SwitchOperator {} } ]
It's not an Observable, right?  false

Additional information:

This code is inspired by an Angular service I was working on. You can think of foo as getting a list of IDs from the backend, and bar and baz being other backend service calls that take IDs.
I don't know if switch is causing the bug, so the title of this issue may need to be updated.

@david-driscoll
Copy link
Member

I'm not really 100% sure what the underlying goal is, however the switch is in the wrong location. And has almost no meaning (zip, is not Observable<Observable<any>>).

function allTogetherNow(): Observable<Payload[]> {
    // Get an Observable of a list.
    const numsObs = foo();

    // Map every item in the Observable list to a new Observable based on
    // the returns of the other two functions.
    return numsObs.map(nums => {
        // For every number in the list:
        return Observable.forkJoin(...nums.map(num => {
            // Return that number as an Observable.
            const numObs = Observable.of(num);
            // Then compute the other two Observables.
            const strObs = bar(num);
            const valObs = baz(num);

            // Zip all three together into the Payload object.
            return Observable.zip(
                numObs, strObs, valObs,
                (myNumber, myString, myObject): Payload =>
                    ({ myNumber, myString, myObject })
                // And then flatten to get the latest result.
            );
        }));
    }).switch();
}

I've made a jsbin with those changes here: http://jsbin.com/teqequsafe/1/edit?js,console,output

The big problem comes in with the array map in the middle, which makes this behavior look non-obvious (I've been bitten by it myself). nums.map(num => { is Array<Observable<T>> as apposed to Observable<Observable<T>>.

The forkJoin in this case forks all the observables until they complete, grabbing the final value. You can always use a different operator here like zip or combineLatest.

@LOZORD
Copy link
Author

LOZORD commented Mar 26, 2017

Thanks for the clarification -- your code does exactly what I want 😄

Basically, make a request that returns Obs<T[]> and map each T to a U, finally resulting in Obs<U[]> (the difficulty being that mapping T to U also uses intermediate Observables).

My understanding of zip is that it is similar to Promise.all. Is that correct?

All that being said, I still believe the types are wrong. Why is my editor telling me that the return type of allTogetherNow is Observable<Payload[]>, when it is provably not?

@david-driscoll
Copy link
Member

Promise.all is more like forkJoin (in that forkJoin waits for completion of all observables)
zip requires all observables emit the same number of items, if they don't then you'll only get the min number of items emitted.
combineLatest/combineAll will combine all values, kind of like zip on steroids.

As for the types remove the explicit type on function allTogetherNow(): Observable<Payload[]> { and it may change. I don't think it will change drastically however since forkJoin by default returns an array.

@kwonoj kwonoj added type: question TS Issues and PRs related purely to TypeScript issues labels Mar 27, 2017
@LOZORD
Copy link
Author

LOZORD commented Mar 29, 2017

Sure, I guess I'm just surprised that the types didn't show the issue with what I had -- that's how I got my initial "solution," fumbling around with the operators until I got the return type I wanted.

rxjs-bad-type

If this doesn't seem like a problem (at least with the types), feel free to close this issue. I'm perfectly happy with your solution.

david-driscoll added a commit to david-driscoll/RxJS-1 that referenced this issue Jun 22, 2017
fixes concatAll, combineAll, exhaust, mergeAll, switch, zipAll.  Also made a few improvements to
how` hot and cold observables are typed.  A few compiler errors were fixed ddue to this change

fixes ReactiveX#2658 ReactiveX#2493 ReactiveX#2551
david-driscoll added a commit to david-driscoll/RxJS-1 that referenced this issue Jun 22, 2017
fixes concatAll, combineAll, exhaust, mergeAll, switch, zipAll.  Also made a few improvements to
how hot and cold observables are typed.  A few compiler errors were fixed due to this change.

fixes ReactiveX#2658 ReactiveX#2493 ReactiveX#2551
david-driscoll added a commit to david-driscoll/RxJS-1 that referenced this issue Jun 22, 2017
fixes concatAll, combineAll, exhaust, mergeAll, switch, zipAll.  Also made a few improvements to
how hot and cold observables are typed.  A few compiler errors were fixed due to this change.

fixes ReactiveX#2658 ReactiveX#2493 ReactiveX#2551
david-driscoll added a commit to david-driscoll/RxJS-1 that referenced this issue Jun 22, 2017
fixes concatAll, combineAll, exhaust, mergeAll, switch, zipAll.  Also made a few improvements to
how hot and cold observables are typed.  A few compiler errors were fixed due to this change.

fixes ReactiveX#2658 ReactiveX#2493 ReactiveX#2551
david-driscoll added a commit to david-driscoll/RxJS-1 that referenced this issue Jun 22, 2017
fixes concatAll, combineAll, exhaust, mergeAll, switch, zipAll.  Also made a few improvements to
how hot and cold observables are typed.  A few compiler errors were fixed due to this change.

fixes ReactiveX#2658 ReactiveX#2493 ReactiveX#2551
david-driscoll added a commit to david-driscoll/RxJS-1 that referenced this issue Jun 23, 2017
fixes concatAll, combineAll, exhaust, mergeAll, switch, zipAll.  Also made a few improvements to
how hot and cold observables are typed.  A few compiler errors were fixed due to this change.

fixes ReactiveX#2658 ReactiveX#2493 ReactiveX#2551
david-driscoll added a commit that referenced this issue Jun 27, 2017
…2690)

fixes concatAll, combineAll, exhaust, mergeAll, switch, zipAll.  Also made a few improvements to
how hot and cold observables are typed.  A few compiler errors were fixed due to this change.

fixes #2658 #2493 #2551
@kwonoj
Copy link
Member

kwonoj commented Jun 29, 2017

Closing via #2690.

@kwonoj kwonoj closed this as completed Jun 29, 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
TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

3 participants