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

Losing type inferences in marble tests #1633

Closed
kwonoj opened this issue Apr 18, 2016 · 16 comments
Closed

Losing type inferences in marble tests #1633

kwonoj opened this issue Apr 18, 2016 · 16 comments
Assignees
Labels
help wanted Issues we wouldn't mind assistance with. TS Issues and PRs related purely to TypeScript issues

Comments

@kwonoj
Copy link
Member

kwonoj commented Apr 18, 2016

RxJS version:
master

Additional information:

Currently to enhance jsdoc test code readability, test scheduler interface is not being directly imported but accessed globally with implicit ambient declaration, so hot or cold observable generated does not support any type inference to Observable<T>

declare const {hot, cold, asDiagram, expectObservable, expectSubscriptions};

var e1 = hot(...);

e1.audit().subscribe() //e1 does not gives any type context

This is somewhat problem since all unit test cases are intended to be used as type definition test cases too, which current test cases are losing its benefit in lot of cases.

@kwonoj kwonoj added the TS Issues and PRs related purely to TypeScript issues label Apr 18, 2016
@kwonoj
Copy link
Member Author

kwonoj commented Apr 18, 2016

/cc @staltz for visibility, also @david-driscoll .

I've quickly tried if it's possible to use ambient declaration to test helper interfaces, but that wasn't successful since dependent types (Observable, Subject, Scheduler, etcs) are also need to be handcrafted since ambient declaration does not allow to import external type definition.

@staltz for asking idea if we can solve this in jsdoc perspective, @david-driscoll for asking type definition perspective I couldn't think of.

@staltz
Copy link
Member

staltz commented Apr 18, 2016

Currently to enhance jsdoc test code readability, test scheduler interface is not being directly imported but accessed globally with implicit ambient declaration

I don't understand what does jsdoc have to do with the marble tests. Can't we make hot() and cold() generic functions hot<T>() and cold<T>()?

@kwonoj
Copy link
Member Author

kwonoj commented Apr 18, 2016

Can't we make hot() and cold() generic functions hot() and cold()?

: no, this isn't about it.

Do you remember original refactoring was importing test helper directly like

import {hot} from ...

hot();

? it's transpiled by compiler something like

var hot_1 = require(...);

hot_1();

so jsdoc test case will display some kind of ugly name. Instead, #1378 introduced to remove explicit import to come over those limitation of jsdoc. Now test cases are not importing explicitly, none of type inferences for generated Observable from hot or cold, type verification / inference including intellisense from editor does not work.

@staltz
Copy link
Member

staltz commented Apr 18, 2016

Oh I see. Can't we declare the types of those functions in each file?

Like

declare hot: string => Observable<any>;

(Just an example, not sure if that compiles)
It would incur some repeated code, on the other hand.

@kwonoj
Copy link
Member Author

kwonoj commented Apr 18, 2016

Can be, but I'm feeling that'll be too much repeated code. (Should import HotObservable, etcs..) I'd consider it as last option. If I can't get feasible solution, I'll go in that way.

@benlesh
Copy link
Member

benlesh commented Apr 19, 2016

Can you build it with a custom typings file that includes these definitions?

@kwonoj
Copy link
Member Author

kwonoj commented Apr 19, 2016

Can you build it with a custom typings file that includes these definitions?

: bit doubt about it. First, can't create external type definition since this issue is result of avoid external import (import ...) for transpiled results. Ambient definition is technically possible, but I found it'll require too much handcraft effort (i.e need to convert most of operator signature into ambient. Maybe I'm missing some easygoing way though).

For now most realistic approach seems @staltz 's suggestion above even it has some amount of duplicated code around test cases, still I'd like to look for other alternative bit more.

@benlesh
Copy link
Member

benlesh commented Apr 19, 2016

Something like this won't do?

"build_spec": "tsc testing-types.d.ts --project ./spec --pretty",

Ambient definition is technically possible, but I found it'll require too much handcraft effort

I'm not sure I'd agree that handcrafting a typings file with ~8 definitions in it is going to be more work than adding definitions to every spec file.

@benlesh
Copy link
Member

benlesh commented Apr 19, 2016

Maybe I'm not understanding the issue here, though.

@kwonoj
Copy link
Member Author

kwonoj commented Apr 19, 2016

handcrafting a typings file with ~8 definitions

: it's not ~8, unfortunately. Let's say one signature as example:

hot(...): HotObservable<T>

now need to create interface

HotObservable<T>: Observable<T>

requires handcraft of Observable<T> as chain. Yes, can't import like import {Observable} .. in ambient type definition unfortunately, TypeScript seems consider it as non-ambient type definition if it contains any external type definition import. Means, all inhertance interface need to be handcrafted along with most operator signature bloating up effort of manual type definition.

Still this is based on my experiment and there might be better way to resolve this.. (:eyes: to @david-driscoll :) ?)

@benlesh
Copy link
Member

benlesh commented Apr 19, 2016

Is this really a "big deal", though? I mean, is this worth all of the trouble?

Is the real answer just to update our test helpers to be a TypeScript module and be more explicit about the usage?

@kwonoj
Copy link
Member Author

kwonoj commented Apr 19, 2016

Is this really a "big deal",

: I'm not considering this as serious problem, though it's bit unfortunate we've moved all test cases into typescript and losing one of benefits for dogfooding type verification.

Is the real answer just to update our test helpers to be a TypeScript module

: That was my original intention and I believe that's simplest solution. What we're trying to solve in here is bending module system in our way to bypass some issues (jsdoc).

I'd consider this as low-priority, good-to-have goals for now. But still believe eventually need to be resolved some way. Best scenario will be documentation system can understand original TS code as-is instead of lean on transpiled results, but that seems not likely to happen with jsdoc.

@kwonoj kwonoj added the help wanted Issues we wouldn't mind assistance with. label Jun 21, 2016
@kwonoj
Copy link
Member Author

kwonoj commented Dec 19, 2016

After experiencing regression issue like #2163 and now library try to follow semver strictly, I started thinking this might need to be looked again.

Originally test case supposed to serve as validating type definitions as well, but currently does not by due to this issue since all of hot/cold obsevable created via test interfaces are simply casted down to any type. We could possibly revisit this issue, or think about other way to guard type definition to not to regress .

@benlesh benlesh added type: discussion and removed help wanted Issues we wouldn't mind assistance with. labels Dec 20, 2016
kwonoj added a commit to kwonoj/rxjs that referenced this issue Dec 20, 2016
kwonoj added a commit to kwonoj/rxjs that referenced this issue Dec 20, 2016
@kwonoj kwonoj self-assigned this Dec 20, 2016
kwonoj added a commit to kwonoj/rxjs that referenced this issue Feb 4, 2017
kwonoj added a commit to kwonoj/rxjs that referenced this issue Feb 9, 2017
@kwonoj
Copy link
Member Author

kwonoj commented Feb 9, 2017

One last thing for this issue is enabling hot(), cold() returns HotObservable<T>, ColdObservable<T> instead of HotObservable<any>, ColdObservable<any> - downcasting to value into any causes to lose some inference like

//this shouldn't work in current type signature with specific type T but casted `any` makes pass this
.filter(x => x) 

Thanksfully default generic will be introduced soon enough (microsoft/TypeScript#2175 (comment)), once we bump up compiler version with supported we can have like HotObservable<T = string> prevent downcasting to any but use string as default, but allow to specify generic if needed.

@kwonoj kwonoj added help wanted Issues we wouldn't mind assistance with. and removed priority: low labels Feb 9, 2017
kwonoj added a commit to kwonoj/rxjs that referenced this issue Feb 9, 2017
@kwonoj
Copy link
Member Author

kwonoj commented Jul 7, 2017

This is somewhat resolved and while there is lot to go, issue itself can be closed. Individual issue can be dealt via PR / separate issue.

@kwonoj kwonoj closed this as completed Jul 7, 2017
@lock
Copy link

lock bot commented Jun 6, 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 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Issues we wouldn't mind assistance with. TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

3 participants