test(filter): enable type inference to marble diagram #2202
Conversation
|
I have assigned @david-driscoll , @staltz as reviewer for code / document side changes just in case. |
|
This is generated doc with PR, which preserves transpiled spec
with type inference supported.
|
| @@ -1,6 +1,12 @@ | |||
| import {expect} from 'chai'; | |||
| import * as Rx from '../../dist/cjs/Rx'; | |||
| declare const {hot, cold, asDiagram, expectObservable, expectSubscriptions}; | |||
| import marbleTestingSignature = require('../helpers/marble-testing'); // tslint:disable-line:no-require-imports | |||
There was a problem hiding this comment.
How about import * as marbleTestingSignature from '../helpers/marble-testing';?
There was a problem hiding this comment.
I haven't tried it but tempted to leave as-is to give clarity this import is unique kind and not being used in general.
There was a problem hiding this comment.
That intent may be better expressed as a comment, not as the usage of require.
There was a problem hiding this comment.
could you not just destructure it with the import?
import {hot, cold, asDiagram, expectObservable, expectSubscriptions} from '../helpers/marble-testing';
There was a problem hiding this comment.
No, it's #1633. If use imported module, TS will transpile it and doc will be affected, reason only use it as type to be removed while transpiling.
There was a problem hiding this comment.
I think a comment here is super important. I can easily see myself wondering why this was done and changing it later.
kwonoj
force-pushed
the
fix-test-type-inference
branch
7 times, most recently
from
ff68a2d to
f802e79
Compare
kwonoj
force-pushed
the
fix-test-type-inference
branch
from
f802e79 to
716377d
Compare
|
@jayphelps @Blesh updated PR with changes to watch PR to validate certain behaviors like large PR, missing type definition import. It'll looks like #2327 (comment)
make status checker fail if those validation does not passes. |
|
🔥 |
|
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 resolves long-pending issue #1633, provides type inference to marble-diagram based test cases while does not hurt documentation for specs. It is using TypeScript compiler's feature to import type signature without real module import and assign it to existing ambient declaration - ambient declaration will provide same type inference to exported modules.
Why Targeting master?
Originally test case were designed to be used as validating type definition as well for most common use cases to prevent issue like #2163 . Due to current setup ergonomics, all of tests are running without any compiler validation so there wasn't any regression testing for type definitions which would be considered as breaking change if occurs.
This PR will enable all test cases to have correct compiler type inference, allows all test cases become first-guard to type definition validation. Once this PR is accepted, I'll work on other test cases to have same changes.
Related issue (if exists):
#1633