Skip to content

[tslint] lint typescript code#19105

Merged
spalger merged 6 commits intoelastic:masterfrom
spalger:implement/tslint
May 22, 2018
Merged

[tslint] lint typescript code#19105
spalger merged 6 commits intoelastic:masterfrom
spalger:implement/tslint

Conversation

@spalger
Copy link
Copy Markdown
Contributor

@spalger spalger commented May 16, 2018

Fixes #18780

Adds TSLint integration.

Running

node scripts/tslint will run the linter on all TypeScript "projects" (defined by tsconfig.json files). This script is also run as a part of testing anywhere eslint is run.

2018-05-17 15 09 08

Rules

In order to avoid lengthy debate about which rules to enable we have enabled:

  • tslint:recommended preset
  • no-unused-variable rule (requires type information so it's not a part of the preset)
  • prettier

We can open issues and have discussions about evolving these selections as we go.

Along with the default rules, a custom rule to ensure that license headers are added to files in the x-pack directory is in use.

To avoid a massive number of changes in this PR tslint.yaml files have been added to the existing TypeScript projects to disable any rules that have violations.

Type Checking

TSLint deprecated the ability to type-check while linting, and it's currently providing a different output than we get with the build because it includes files that are not shipped with the build. Rather than block TypeScript integration on this I discussed the options with @azasypkin and we both agree that the type checking done at build time is sufficient as a final check that must pass before files can be committed, that editor integrations are okay for development, and that people can run the tsc directly if they want more comprehensive type checking during development:

yarn tsc --noEmit --watch --pretty --project ./relative/path/to/tsconfig.json

@spalger spalger force-pushed the implement/tslint branch from 6ec4110 to ff5cef4 Compare May 16, 2018 02:08
@spalger spalger force-pushed the implement/typescript branch from d0e0866 to 6811044 Compare May 16, 2018 02:09
@spalger spalger changed the base branch from implement/typescript to master May 16, 2018 02:10
@spalger spalger force-pushed the implement/tslint branch 5 times, most recently from 96ccc5b to 8d8b66b Compare May 17, 2018 22:16
@spalger spalger force-pushed the implement/tslint branch from 8d8b66b to 890af3f Compare May 21, 2018 14:56
@spalger spalger force-pushed the implement/tslint branch from a0eae44 to 2c0e40d Compare May 21, 2018 15:42
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

for (const Linter of [Eslint, Tslint]) {
const filesToLint = Linter.pickFilesToLint(log, files);
if (filesToLint.length > 0) {
await Linter.lintFiles(log, filesToLint);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current implementation doesn't attempt to run tslint at all if there is an error from eslint, so if you have lint failures in both .js and .ts files, you won't know that until you attempt to commit again after fixing the .js files. It should probably run both commands and show the results from each.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the same is true for file casing issues and eslint. If there are casing issues eslint is never run.

I'll put up a separate PR that updates the precommit hook to clearly log errors from multiple tasks and collect those errors into an appropriate exit code, but I personally think it's out of the scope of this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weltenwort
Copy link
Copy Markdown
Member

I failed to get this to pick up projects located in ../kibana-extra/* even though they are listed in the kibana.dev.yml. Is this outside of the scope of this tool or do I need to change the invocation in some way?

@spalger
Copy link
Copy Markdown
Contributor Author

spalger commented May 22, 2018

@weltenwort just like our current eslint setup this isn't intended to lint things in ../kibana-extra/*. It's not an unrealistic feature request though...

Copy link
Copy Markdown
Member

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@spalger
Copy link
Copy Markdown
Contributor Author

spalger commented May 22, 2018

jenkins, test this

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@spalger spalger merged commit 4c2a90d into elastic:master May 22, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request May 22, 2018
* [tslint] lint typescript code

* [tslint] filter projects when running specific project

* [dev/ts] use more explicit types

* [dev/ts] add note about why using glob

* [dev/ts] rely on ts, use fewer getters
spalger pushed a commit that referenced this pull request May 22, 2018
Backports the following commits to 6.x:
 - [tslint] lint typescript code  (#19105)
@spalger
Copy link
Copy Markdown
Contributor Author

spalger commented May 22, 2018

6.x/6.4: 37487a8

@spalger spalger deleted the implement/tslint branch May 22, 2018 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants