Skip to content

chore: Adopt no-unused-vars-experimental for eslint to prevent incorrect linting errors #981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 6, 2020

Conversation

MathBunny
Copy link
Contributor

Context

There are several occasions where eslint incorrectly flags code claiming it violates the @typescript-eslint/no-unused-vars rule, for example while exporting namespaces that don't have any self-referencing in the typings files.

We can adopt no-unused-vars-experimental, which "leverages the TypeScript compiler's unused variable checks to report", and seemingly doesn't fall victim to the previous incorrect claims.

Approach

Simply enable the experimental rule and disable the recommended rule. However, once doing that the following error would appear:

Error: Error while loading rule '@typescript-eslint/no-unused-vars-experimental':
 You have used a rule which requires parserServices to be generated. 
You must therefore provide a value for the "parserOptions.project" property 
for @typescript-eslint/parser.

To resolve this error, the following can be added:

"parserOptions": {
    "project": "./tsconfig.json"
  }

However, I realized later that not all files are mentioned in the current tsconfig.json. Apart from adding an include with a glob adding all src/test files, another option is to create a dedicated config just for linting, which is the approach taken here.

Another option was createDefaultProgram: true, but that has complexity O(n^m), n = # files not included in config, m = average depth of the dependency tree of those respective files. I ran it with this setting and found it took ~1 minute to lint on my local computer so far from ideal.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM with a nit.

@hiranya911 hiranya911 assigned MathBunny and unassigned hiranya911 Aug 6, 2020
@MathBunny
Copy link
Contributor Author

Commenting for visibility the performance impact this has even without including createDefaultProgram:

Rule                                           | Time (ms) | Relative
:----------------------------------------------|----------:|--------:
@typescript-eslint/no-unused-vars-experimental |  3164.744 |    57.1%
indent                                         |  1709.654 |    30.8%
keyword-spacing                                |   137.947 |     2.5%
@typescript-eslint/camelcase                   |   120.434 |     2.2%
@typescript-eslint/type-annotation-spacing     |    49.622 |     0.9%
@typescript-eslint/no-empty-function           |    49.448 |     0.9%
no-regex-spaces                                |    25.818 |     0.5%
no-unexpected-multiline                        |    22.458 |     0.4%
no-mixed-spaces-and-tabs                       |    21.964 |     0.4%
object-curly-spacing                           |    17.590 |     0.3%
Rule                                           | Time (ms) | Relative
:----------------------------------------------|----------:|--------:
@typescript-eslint/no-unused-vars-experimental |  7122.536 |    60.3%
indent                                         |  3529.941 |    29.9%
@typescript-eslint/camelcase                   |   208.376 |     1.8%
@typescript-eslint/no-empty-function           |   205.101 |     1.7%
keyword-spacing                                |   128.697 |     1.1%
no-unexpected-multiline                        |   103.078 |     0.9%
no-regex-spaces                                |    72.967 |     0.6%
object-curly-spacing                           |    53.535 |     0.5%
no-useless-escape                              |    34.520 |     0.3%
no-control-regex                               |    29.842 |     0.3%

This is for src/test respectively. Moreover, we have the following benchmarks for former/PR/createDefaultProgram:

  • 13.43 real 26.92 user 1.75 sys
  • 24.21 real 55.85 user 3.47 sys
  • 111.13 real 190.30 user 13.43 sys

@MathBunny MathBunny merged commit 42fac27 into master Aug 6, 2020
@MathBunny MathBunny deleted the hlazu-eslint-no-unused-vars-experimental branch August 6, 2020 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants