[test] Lint typescript definitions #12637
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While trying to to run all the *.tsx? files might show the correct usage of this package with typescript it does not provide a complete test suite for the definitons i.e. we cannot write tests that expect an error. The longterm goal is to fully extends dtslint rules and make use of the $Expect* helpers to display where typescript can help us avoid errors at compile time. It will also enable linting for our definitons files.
Started incrementally adding rules to tslint until we ideally satisfy dtslint and tslint-react
Flow got introduced accidentally while rebasing
Seems like a yarn issue were adding a dependency includes the github libk but running yarn again causes yarn to remove the github prefix.
pelotom
approved these changes
Aug 24, 2018
| @@ -49,7 +49,7 @@ | |||
| "test:coverage:html": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha packages/material-ui/test/**/*.test.js packages/material-ui/src/{,**/}*.test.js && nyc report --reporter=html", | |||
| "test:karma": "cross-env NODE_ENV=test karma start test/karma.conf.js --single-run", | |||
| "test:regressions": "webpack --config test/regressions/webpack.config.js && rimraf test/regressions/screenshots/chrome/* && vrtest run --config test/vrtest.config.js --record", | |||
| "typescript": "tsc -p tsconfig.json", | |||
| "typescript": "tslint -p tsconfig.json \"packages/*/{src,test}/**/*.{ts,tsx}\"", | |||
There was a problem hiding this comment.
Is it intentional that you're not actually using dtslint yet?
There was a problem hiding this comment.
Kind of by accident if that makes sense. This just means that we are only using the rules from dtslint.
dtslint requires some extra setup because it can't run on more than one package on its own. I was wondering why this worked so seamless all of the sudden.
I guess this is good for now since we do not use $ExpectError and target multiple typescript versions which does require dtslint.
There was a problem hiding this comment.
I added it as an explicit dependency for now to avoid further confusion.
Since we are using tslint as the lint runner we should list it explicitly.
oliviertassinari
approved these changes
Aug 24, 2018
Closed
marcelpanse
pushed a commit
to marcelpanse/material-ui
that referenced
this pull request
Oct 2, 2018
* [test] Use tslint instead of typescript to test definitions While trying to to run all the *.tsx? files might show the correct usage of this package with typescript it does not provide a complete test suite for the definitons i.e. we cannot write tests that expect an error. The longterm goal is to fully extends dtslint rules and make use of the $Expect* helpers to display where typescript can help us avoid errors at compile time. It will also enable linting for our definitons files. * [tslint] Enable no-unnecessary-type-assertion rule Started incrementally adding rules to tslint until we ideally satisfy dtslint and tslint-react * [tslint] Enable no-unnecessary-callback-wrapper rule * [tslint] Enable member-access rule * [tslint] Enable space-within-parens rule * [tslint] Enable file-name-casing rule * [tslint] Enable comment-format rule * [tslint] Enable semicolon rule * [tslint] Enable interface-name rule * [tslint] Fix errors reintroduced when rebasing * [tslint] Enable strict-export-declare-modifiers rule * [tslint] Enable no-relative-import-in-test rule * [tslint] Enable use-default-type-parameter rule * [tslint] Disable no-unnecessary-generics rule * [tslint] Enable prefer-declare-function rule * [tslint] Enable interface-over-type-literal rule * [tslint] Enable array-type rule * [tslint] Enforce ban-types rule * [tslint] Enable no-duplicate-imports rule * [tslint] Enable callable-types rule * [test] Use tslint from project root * [test] Move lint to typescript test command * [test] Use typescript test in CI Flow got introduced accidentally while rebasing * [tslint] Enable strict mode * [tslint] Remove unused suppressImplicitAnyIndexErrors * [test] Run prettier * [core] Run yarn Seems like a yarn issue were adding a dependency includes the github libk but running yarn again causes yarn to remove the github prefix. * [test] Add tslint as an explicit dependency Since we are using tslint as the lint runner we should list it explicitly. * [tslint] Squash rules that are already set in dtslint
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While trying to to run all the *.tsx? files through the compiler might show the correct usage of this package with typescript it does not provide a complete test suite for the definitons i.e. we only have positive assertions but no negative.
This PR is the first step for providing negative assertions to the typescript tests. By adding
dtslintto the CI pipeline we enable linting for the definition and test files and follow the standard for definition files set by DefinitelyTyped. I deliberately added not more than one rule per commit to make review easier. If we decide against certain rules we can just remove the commit from this PR. If we decided on a ruleset I would squash the rules into a singleextends: ["dtslint/dtslint.json"]and leave only our exceptions in thetslint.json.I disabled
no-unnecessary-genericsbecause most cases are not that obvious in my opinion and without good test coverage might break things if we'd remove the generics.This PR also enables strict mode in typescript and removes the
suppressImplicitAnyIndexErrorsoption. Both changes did not cause any errors and I think strict mode should be our goal.The next step should be to improve typescript tests by adding
$ExpectTypeand$ExpectErrorthroughout the component test. I'd suggest adding the tests next to the file as is already done with the js tests. The single component test file inmaterial-ui/test/typescriptis very unwieldy. I actually like theFILE.spec.tsschema. It makes a clearer distinction betweenjsandtstests i.e. run time vs compile time test.The issues with
dtslintoriginally described in #12332 were mostly resolved with the switch to[email protected]. Per package tsconfig support is still poor withdtslintbecause it does not respectextendsproperties. I'm working on a fix for that./cc @pelotom