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

Observable<never> is not assignable to Subscribable<T> #3891

Closed
felixfbecker opened this issue Jul 4, 2018 · 3 comments · Fixed by #4050 or severest/retrobot#221
Closed

Observable<never> is not assignable to Subscribable<T> #3891

felixfbecker opened this issue Jul 4, 2018 · 3 comments · Fixed by #4050 or severest/retrobot#221
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@felixfbecker
Copy link
Contributor

Bug Report

Current Behavior

Kinda related to #3890, when trying implement interop by accepting Subscribable as input, you cannot pass Observable<never> into any Subscribable<T>.
This is because Subscribable's signature for subscribe doesn't match that of Observable:

    subscribe(observerOrNext?: PartialObserver<T> | ((value: T) => void), error?: (error: any) => void, complete?: () => void): Unsubscribable;

vs

    subscribe(observer?: PartialObserver<T>): Subscription;
    subscribe(next?: (value: T) => void, error?: (error: any) => void, complete?: () => void): Subscription;

So you get this error:

Argument of type 'Observable<never>' is not assignable to parameter of type 'Subscribable<number>'.
  Types of property 'subscribe' are incompatible.
    Type '{ (observer?: NextObserver<never> | ErrorObserver<never> | CompletionObserver<never> | undefined)...' is not assignable to type '(observerOrNext?: NextObserver<number> | ErrorObserver<number> | CompletionObserver<number> | ((v...'.
      Types of parameters 'observer' and 'observerOrNext' are incompatible.
        Type 'NextObserver<number> | ErrorObserver<number> | CompletionObserver<number> | ((value: number) => v...' is not assignable to type 'NextObserver<never> | ErrorObserver<never> | CompletionObserver<never> | undefined'.
          Type '(value: number) => void' is not assignable to type 'NextObserver<never> | ErrorObserver<never> | CompletionObserver<never> | undefined'.
            Type '(value: number) => void' is not assignable to type 'CompletionObserver<never>'.
              Property 'complete' is missing in type '(value: number) => void'.

Reproduction

import { Observable, Subscribable, NEVER } from 'rxjs'

function foo(input: Subscribable<number>) {}

foo(NEVER)

Expected behavior

An Observable that never emits should be assignable to any Observable of any type, since the type doesn't matter if it never emits. This is the semantic of never.

Environment

  • Runtime: any
  • RxJS version: 6.2.1

Possible Solution
Change the signature of Subscribable.subscribe to use an overload, which in my eyes more accurately represents the actual interface.
With the current signature, this is possible, which it shouldn't:

subscribable.subscribe({ error: err => console.log('err 1') }, err => console.log('err 2'))

What would get logged?

@felixfbecker felixfbecker changed the title NEVER is not assignable to Subscribable<T> Observable<never> is not assignable to Subscribable<T> Jul 4, 2018
@cartant
Copy link
Collaborator

cartant commented Jul 4, 2018

This looks like a bit of a problem, as the signature for subscribe in the TC39 proposal - to which Symbol.observable refers - accepts only an observer. So typing it otherwise would be a lie, basically.

@felixfbecker
Copy link
Contributor Author

Removing the positional parameters solves the problem too

@cartant
Copy link
Collaborator

cartant commented Jul 4, 2018

@felixfbecker Ah, I see what you mean. It's already lying.

Also, regarding the WHATWG proposal, I've not (re)read the entire thread, but this comment suggests that its implementation of subscribe will also take only a single parameter: the observer.

The signature of Subscribable should probably represent that.

@kwonoj kwonoj added the TS Issues and PRs related purely to TypeScript issues label Jul 10, 2018
felixfbecker added a commit to felixfbecker/rxjs that referenced this issue Aug 23, 2018
benlesh pushed a commit that referenced this issue Aug 27, 2018
* fix(subscribable): make subscribe() signature match Observable

Fixes #3891

* test: cast string | number to string before concatenating

T of this Observable is string | number, so TypeScript does not allow
using +. The desired behaviour for this function is to concatenate the
string values, so cast to strings. On master, these were incorrectly
inferred as never, which is why this error was not surfaced.

* test: correct type for selector function

The hot() function will create an Observable that emits strings from
marble diagrams (even if the strings contain numbers).
So the type of x should be string, not number.
@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 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
3 participants