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

fix(typings): fix subscribe overloads #3053

Merged
merged 1 commit into from
Dec 1, 2017
Merged

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Nov 6, 2017

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
Copy link

rxjs-bot commented Nov 6, 2017

Messages
📖

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

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

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

@benlesh
Copy link
Member

benlesh commented Nov 8, 2017

Can we add typings tests for this?

@cartant
Copy link
Collaborator Author

cartant commented Nov 8, 2017

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
Copy link
Member

benlesh commented Nov 9, 2017

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
Copy link
Member

benlesh commented Nov 9, 2017

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

@benlesh benlesh changed the base branch from master to stable November 9, 2017 18:38
@benlesh
Copy link
Member

benlesh commented Nov 9, 2017

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

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 ReactiveX#3052
@cartant
Copy link
Collaborator Author

cartant commented Nov 9, 2017

@benlesh Rebased.

@cartant
Copy link
Collaborator Author

cartant commented Nov 10, 2017

@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
Copy link
Member

kwonoj commented Nov 10, 2017

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

@benlesh
Copy link
Member

benlesh commented Nov 28, 2017

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
Copy link
Collaborator Author

cartant commented Nov 28, 2017

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
Copy link
Member

kwonoj commented Nov 28, 2017

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

@coveralls
Copy link

coveralls commented Nov 28, 2017

Coverage Status

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

@benlesh benlesh merged commit 1a9fd42 into ReactiveX:stable Dec 1, 2017
@cartant cartant deleted the issue-3052 branch March 31, 2018 02:28
@lock
Copy link

lock bot commented Jun 5, 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 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants