test(deferObservable): add type definition test #1479
Conversation
| @@ -72,4 +72,16 @@ describe('Observable.defer', () => { | |||
| expectObservable(e1, unsub).toBe(expected); | |||
| expectSubscriptions(source.subscriptions).toBe(sourceSubs); | |||
| }); | |||
|
|
|||
| type(() => { | |||
There was a problem hiding this comment.
This might be the wrong time to point this out... but I have NO IDEA what these tests do. I find them almost unreadable.
There was a problem hiding this comment.
I don't think there's wrong time, or even it'd be great time since we don't have much test for this yet. :)
Basically this test is ensuring type definition is 'works' as it's actual functionality does, for instance in case of this one defer, type should support cases
- when factory function returns
observable<T>, subscribe should carry over type T so you can access its properties without type errors - factory function can throw without return anything, so defer should accept function throws but does not return
both of declaration in line 82 / 83 illustrates those usecases, and since it's compile time it is being verified when test case is being compiled. If those are not working as intended, compiler will throw out messages of expected type vs. provided one.
I also agree it could be possibly confusing to consume this kind of test (small one would be ok, but case like this? https://github.com/ReactiveX/RxJS/pull/1189/files) and it'd be great if there's suggestion how to improve this or requirement should be added for better visibility. Since there is barely no test yet, changes can be easily introduced.
There was a problem hiding this comment.
it seems like it would be better just to work that into the TypeScript of our actual tests. Since those will cause the compilation of the spec file themselves to fail and thus cause the tests to fail. Do we really need an additional test that doesn't seem to do anything once it's transpiled?
There was a problem hiding this comment.
feasible opinion. In most cases test cases already consumes type definitions, so probably won't need to be covered once again via explicit. I remember few cases that test case does not include all of possible type definition cases though - start with opt out and only add necessary cases once it's being noticed maybe better way to go?
There was a problem hiding this comment.
overloaded signature like combinelatest (#1189) might be few edge cases need these kind of test cases.
|
Let me not check this in to have better proposed PR for typing tests. /cc @david-driscoll also assume he might be interested as well. |
|
I have closed this PR without merging in favor of #1484. |
|
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. |
Description:
This PR adds type definition test for
deferObservable, as well as amending parameter type to allow function can throw without return value.Related issue (if exists):
Additional
Issue at Typescript (microsoft/TypeScript#1042) will allow non-void function can throw once it's arrived.