fix(typings): fix subscribe overloads

[cartant]

Remove the no-arg overload. If not removed, any Observable will be compatible with any ObservableInput regardless of type - as T does not appear in the no-arg overload.

Closes #3052

Description:

The overload signatures for subscribe include a signature that takes no parameters. This is a problem and results in any Observable being deemed compatible with any ObservableInput.

For example, this code will compile without error:

import { Observable, ObservableInput } from "rxjs/Observable";
import "rxjs/add/observable/of";

const numbers = Observable.of<number>(1);
const strings = Observable.of<string>("1");

let input: ObservableInput<string>;
input = numbers;
input = strings;

The problem is caused by T not appearing in the signature. The signature should be removed and the observer in the subsequent signature should be made optional. With that change, the compilation of the above code fails with this error:

error TS2322: Type 'Observable<number>' is not assignable to type 'ObservableInput<string>'.
  Type 'Observable<number>' is not assignable to type 'ArrayLike<string>'.
    Property 'length' is missing in type 'Observable<number>'.

Related issue (if exists): #3052

[rxjs-bot]

	Messages
📖

CJS: 2278.8KB, global: 746.2KB (gzipped: 120.0KB), min: 145.7KB (gzipped: 31.2KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage remained the same at 97.204% when pulling db260f2 on cartant:issue-3052 into a922087 on ReactiveX:master.

[benlesh]

Can we add typings tests for this?

[cartant]

We can, but I don't think they'll test anything useful.

What would be much better would be to add some snippet-based testing that could test for failures. It really needs a test to ensure that an Observable<string> cannot be assigned to an ObservableInput<number>. That's not possible with the basic type test mechanism, as the entire compilation will fail.

Some snippet-based testing would be useful for testing other overloads, too. I discussed this with OJ in another issue - from this comment onwards.

For another example of what the snippet tests would look like, have a look at research-spec.ts in my ts-action repo.

If adding snippet testing infrastructure to RxJS is something you'd find agreeable, I can look into it.

[benlesh]

Okay... LGTM. I think that we should have a longer conversation about type testing. Perhaps with the TypeScript folks. I'm very interested in your solution, but it seems like TypeScript should provide more in this area. cc @david-driscoll

[benlesh]

@kwonoj do you think we should merge this into stable and cherry pick them over to master? I think so, personally.

[benlesh]

@cartant can you please rebase this against stable rather than master so we get get it out in the next patch?

[cartant]

@benlesh Rebased.

[cartant]

@benlesh

it seems like TypeScript should provide more in this area

The library is a fairly thin wrapper around TypeScript's Language Service API. In fact, it's pretty much based on the example in the referenced documentation. There are other snippet compiler libraries/tools out there, but I wasn't able to find one that supported importing modules.

[kwonoj]

@benlesh yeah, seems non breaking change to me go for stable.

[benlesh]

Travis is failing for some reason on this can we figure out why? I'm going to rerun it. I'm looking at this from my phone so I couldn't really see the logs.

[cartant]

It appears to be breaking because of the types that are installed for lodash:

4256             ...props: Array<Many<keyof T>>
                                            ~
node_modules/@types/lodash/index.d.ts(4256,40): error TS1005: '>' expected.

keyof was introduced in TypeScript 2.1 and the stable package.json uses TypeScript "~2.0.6".

I was able to get the tests to pass on Travis on my branch by clearing Travis's cache. How it gets into the state that it's currently in, I'm not sure. But it does seem cache-related.

[kwonoj]

flushed cache and retriggered build, also created #3132 for better module cache.

[coveralls]

Coverage remained the same at 97.204% when pulling db260f2 on cartant:issue-3052 into a922087 on ReactiveX:master.

[lock]

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.