Losing type inferences in marble tests

[kwonoj]

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]

/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]

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]

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]

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]

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]

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

[kwonoj]

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]

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]

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

[kwonoj]

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]

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]

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]

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 .

[kwonoj]

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]

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.

[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.