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

Mixed types in pipes with mergeMap and more than 8 parameters yield typescript error #3766

Closed
akehir opened this issue May 31, 2018 · 8 comments
Labels
bug Confirmed bug TS Issues and PRs related purely to TypeScript issues

Comments

@akehir
Copy link
Contributor

akehir commented May 31, 2018

In the below code, foo1() is ok, but foo2() is not. In other words, in pipes with more than 8 parameters in the stream, the type definition assumes that all the observables of the stream return the same types.

RxJS version:
6.2.0

Code to reproduce:
Stackblitz: https://stackblitz.com/edit/typescript-e8dcm5?file=index.ts

import {
  of,
  Observable,
} from 'rxjs';

import {
  tap,
  mergeMap,
} from 'rxjs/operators';

export class Foo {

  public foo1(): Observable<boolean> {
    return of(true).pipe(
      tap(), // 1
      tap(), // 2
      tap(), // 3
      tap(), // 4
      tap(), // 5
      tap(), // 6
      tap(), // 7
      mergeMap(() => {
        return of('');
      }),
      mergeMap(() => {
        return of(true);
      }),
    );
  }

  public foo2(): Observable<boolean> {
    return of(true).pipe(
      tap(), // 1
      tap(), // 2
      tap(), // 3
      tap(), // 4
      tap(), // 5
      tap(), // 6
      tap(), // 7
      tap(), // 8
      mergeMap(() => {
        return of('');
      }),
      mergeMap(() => {
        return of(true);
      }),
    );
  }
}

Expected behavior:
No typescript compilation error, as mergeMap should be able to change the type of the Observable, and only the last mergeMap should be relevant for the resulting type.

Actual behavior:

ERROR in index.ts(41,7): error TS2345: Argument of type 'OperatorFunction<boolean, string>' is not assignable to parameter of type 'OperatorFunction<boolean, boolean>'.
  Type 'string' is not assignable to type 'boolean'.

Additional information:
The issue seems to be with Observable.ts, line 292, and mergeMap.ts, line 13:

pipe<R>(...operations: OperatorFunction<T, R>[]): Observable<R>;
export function mergeMap<T, R>(project: (value: T, index: number) => ObservableInput<R>, concurrent?: number): OperatorFunction<T, R>;

With more than 8 parameters, apparently all the OperatorFunction<T, R> need to return the same type .

@cartant
Copy link
Collaborator

cartant commented May 31, 2018

That particular signature for pipe is incorrect. It most likely should be:

pipe<R>(...operations: OperatorFunction<any, any>[]): Observable<R>;

as there's no way each operator can take an observable of type T and return one of type R.

@akehir
Copy link
Contributor Author

akehir commented May 31, 2018

Ok, the code would need to change to the below - but with the current signature for pipe the typescript compilation fails nonetheless.

public foo2(): Observable<boolean> {
    return of(true).pipe<boolean>(
      tap(), // 1
      tap(), // 2
      tap(), // 3
      tap(), // 4
      tap(), // 5
      tap(), // 6
      tap(), // 7
      tap(), // 8
      mergeMap(() => {
        return of('');
      }),
      mergeMap(() => {
        return of(true);
      }),
    );
  }

@cartant
Copy link
Collaborator

cartant commented May 31, 2018

The current signature for pipe is the problem, that's why the compilation fails. Any, yes, it will be necessary to explicitly specify the R type parameter in the call.

@benlesh benlesh added the bug Confirmed bug label Jun 4, 2018
@benlesh
Copy link
Member

benlesh commented Jun 4, 2018

Labeling this as a bug, just because of this comment

@benlesh benlesh added the TS Issues and PRs related purely to TypeScript issues label Jun 4, 2018
@kjmcclain
Copy link

This overload on pipe:

pipe<R>(...operations: OperatorFunction<any, any>[]): Observable<R>;

causes type checking to break because:

Observable<T> o1;
Observable<string> o2 = o2.pipe(map((x : X) => x + ''));

type checks, though it shouldn't. (It matches that overload.) (I noticed that in version 6.2.2)

I'm very new to rxjs and typescript too, but it would seem that the only the correct type for operations in the overload would be something like (and let me make up notation) OperatorFunction*<T, R>, where for any binary type-function F, F*<A, B> is the type of paths from A to B in the graph defined with vertexes being types and the edges between two types A and B being the values of type F<A, B>. (Here, a path is just a list of edges that "connect up" at the vertexes.) I doubt typescript has such a type but since it is the only correct type, I'd just remove this overload.

I've seen some other bugs here that people complain about using the spread operator with pipe, eg, like o.pipe(...operations). I wouldn't support this for the same reason that typescript can't type check it. I don't know their use case, but it would seem that if they were passing a list of operations around, they could just instead pass a single typed operation around, since any number of operations can be "compressed" into a "single" operation by using the static pipe.

As for the case in this bug, more than eight operations explicitly passed, I'd just recommend chaining or using the static pipe:

o.pipe(o1, o2, o3, o4).pipe(o5, o6, o7, o8, o9)
// or
o.pipe(pipe(o1, o2, o3, o4), pipe(o5, o6, o7, o8, o9))

It might be a drag to not have the ...operations overload defined, but I'd much rather have type checking work for eight or less arguments since that quite common.

@cartant
Copy link
Collaborator

cartant commented Aug 27, 2018

@kjmcclain Your comment - and this issue itself - should have been addressed by #3945, which has been merged, but there isn't yet a release that includes the change.

@cartant
Copy link
Collaborator

cartant commented Aug 27, 2018

Closing this, as it's been dealt with in #3789 and #3945.

@cartant cartant closed this as completed Aug 27, 2018
@kjmcclain
Copy link

Awesome, thanks! I tried searching for an open issue about this, but I couldn't find it.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Confirmed bug TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

4 participants