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

switchAll, mergeAll etc are not type safe #3841

Closed
felixfbecker opened this issue Jun 14, 2018 · 5 comments · Fixed by severest/retrobot#221
Closed

switchAll, mergeAll etc are not type safe #3841

felixfbecker opened this issue Jun 14, 2018 · 5 comments · Fixed by severest/retrobot#221
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@felixfbecker
Copy link
Contributor

Bug Report

Current Behavior

const input: Observable<number> = of(1, 2, 3)
const output = input.pipe(switchAll())
output.subscribe(value => console.log(value))

No type error, but runtime error:

You provided '1' where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.

Reproduction

Expected behavior
This should give a type error like number is not assignable to ObservableInput<T>

Environment

  • RxJS version: 6.2.1

Possible Solution
I tried a few things, but it seems like type inference just fails here. TypeScript needs to infer a type based on the usage of the return value here.

Workaround
Use switchMap(x => x) instead (switchMap(identity) also breaks inference)

@cartant
Copy link
Collaborator

cartant commented Jun 14, 2018

The problem seems to have been introduced in 6.2.1 with this commit - which uses the signature that I suggested.

Prior to that commit, the snippet would have effected this error:

[ts]
Argument of type 'OperatorFunction<ObservableInput<{}>, {}>' is not assignable to parameter of type 'OperatorFunction<number, {}>'.
  Type 'number' is not assignable to type 'ObservableInput<{}>'.

The commit fixed an error with the pipe signature, but it could be improved. A more refined rest-parameters signature for pipe would be:

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

Such a signature would avoid matching the first parameter with any, but, unfortunately, that's as far as this approach could be taken. It would not solve the problem if switchAll where, say, the second of two parameters. For that situation, a signature like this would be required:

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

However, such a signature requires the inclusion of another type parameter. And given that the caller of such a rest-parameter signature will want to explicitly specify R, the signature isn't really usable.

I don't know if it's going to be possible to support a rest-parameters signature with which the caller can specify only the result type (R) and proper type safety for the parameters.


As an aside, this is a regression that would have been caught with an appropriate dtslint test - see #3823. Something like this would have caught it:

const input: Observable<number> = of(1, 2, 3);
const output = input.pipe(switchAll()); // $ExpectError

@felixfbecker
Copy link
Contributor Author

I don't like the idea of degrading type safety for common cases like this just to support more than n parameters to pipe(). FWIW, the TypeScript compiler cheats in many places itself, e.g. will only check circular types n levels deep (7 I think). I would rather have pipe() only take a maximum of n parameters (let's say 10) than have type safety silently degrade. A user can always call pipe() twice.

That said, would microsoft/TypeScript#24897 eliminate the need for all the overloads? We should definitely try that out

@cartant
Copy link
Collaborator

cartant commented Jun 14, 2018

I've seen the TypeScript PR that you've referenced and looking more closely at it is on my TODO list. However, I suspect that it won't help in the pipe case because of the input-output relationship that exists between consecutive parameters. Hopefully, I'm wrong.

@cartant
Copy link
Collaborator

cartant commented Jun 15, 2018

My preferred solution is to abandon the only-specifiy-R signature and add another signature that appends the rest parameter to a parameters-up-to-I signature.

That is, remove this:

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

And add this:

pipe<A, B, C, D, E, F, G, H, I, R>(
  op1: OperatorFunction<T, A>,
  op2: OperatorFunction<A, B>,
  op3: OperatorFunction<B, C>,
  op4: OperatorFunction<C, D>,
  op5: OperatorFunction<D, E>,
  op6: OperatorFunction<E, F>,
  op7: OperatorFunction<F, G>,
  op8: OperatorFunction<G, H>,
  op9: OperatorFunction<H, I>,
  ...operations: OperatorFunction<any, any>[]
): Observable<R>;

Instead of specifying a single R type parameter, callers would have to apply a type assertion, like this:

const result = of("T").pipe(
    mapTo("A"),
    mapTo("B"),
    mapTo("C"),
    mapTo("D"),
    mapTo("E"),
    mapTo("F"),
    mapTo("G"),
    mapTo("H"),
    mapTo("I"),
    mapTo("R")
) as Observable<"R">; // Otherwise inferred to be Observable<{}>

@benlesh
Copy link
Member

benlesh commented Jun 18, 2018

Overall, the typing around the pipe method has been perpetually problematic.

Perhaps we can get @ahejlsberg or @mhegazy's attention and this can be solved either by helping us get it right, or with some solution to the problem on the TypeScript side of things.

@benlesh benlesh added the TS Issues and PRs related purely to TypeScript issues label Jun 18, 2018
cartant added a commit to cartant/rxjs that referenced this issue Jul 23, 2018
Replace the rest parameters overload with a signature that also includes
the A-I type parameters.

Closes ReactiveX#3823 ReactiveX#3841
cartant added a commit to cartant/rxjs that referenced this issue Jul 23, 2018
Replace the rest parameters overload with a signature that also includes
the A-I type parameters.

Closes ReactiveX#3841
cartant added a commit to cartant/rxjs that referenced this issue Jul 24, 2018
Replace the rest parameters overload with a signature that also includes
the A-I type parameters.

Closes ReactiveX#3841
cartant added a commit to cartant/rxjs that referenced this issue Jul 25, 2018
Replace the rest parameters overload with a signature that also includes
the A-I type parameters.

Closes ReactiveX#3841
@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 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

Successfully merging a pull request may close this issue.

3 participants