Skip to content
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

chore: reconfigure ESLint build, improve lint speed #4078

Merged
merged 30 commits into from
Apr 21, 2020

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Apr 21, 2020

  • Rename @blueprintjs/eslint-plugin-blueprint to @blueprintjs/eslint-plugin for brevity. The last part of the name was redundant, and ESLint will resolve the package correctly without it. This required changing some inline enable/disable code flags.
  • Remove a config option (createDefaultProgram) added in Fix failure to run eslint on win platform #4060 which was greatly slowing down ESLint. We don't need to create a default program for each file encountered; we can be explicit about what is in the TS program.
  • Be more explicit about ESLint's working directories
  • Remove tslint-plugin-prettier from @blueprintjs/tslint-config
  • Install eslint-plugin-prettier in @blueprintjs/eslint-config
    • Avoid running eslint-plugin-prettier in CI, but continue to run it in the default case so that vscode-eslint can report & fix errors for eslint and prettier in a single pass (we do this because vscode doesn't run multiple formatters for a filetype, and it is cumbersome to explicitly have to run both formatters while coding).

During this process I did some very rough profiling of different lint configurations on my Macbook. This is for linting the whole monorepo with lerna run lint:es --parallel:

Lint task Type info? Run prettier? createDefaultProgram: true Time (s)
tslint no no 8
eslint (w/ eslint-plugin-tslint) no no no 8
eslint typescript-eslint no no 21
eslint (w/ eslint-plugin-tslint) typescript-eslint no no 26.7
eslint + tslint typescript-eslint no no 31
tslint ts no 32
eslint (w/ eslint-plugin-tslint) typescript-eslint eslint-plugin-prettier no 42.7
eslint (w/ eslint-plugin-tslint) typescript-eslint eslint-plugin-prettier yes 72
eslint + tslint typescript-eslint eslint-plugin-prettier yes 78

From these performance characteristics I decided that:

  • running TSLint rules through ESLint with eslint-plugin-tslint does not cause a significant performance impact, so we should continue doing it
  • running prettier through ESLint has a moderate impact, so we should avoid doing it when possible and use the typescript-eslint team's advice of creating a yarn format command to run prettier. in cases where convenience outweighs the performance cost (vscode-eslint), we should continue to run eslint-plugin-prettier.

There are a bunch of whitespace formatting changes in this PR (you can view the diff without them here). Some of them are in .md files which were never formatted before, others are in test/ folders where tslint hadn't been running properly before. Outside of those files I'm not 100% sure why there are code diffs on line break formatting. I've kept the prettier version the same in this PR; the next one will include the bump to v2.0.

@blueprint-bot
Copy link

revert a very dumb autofix by blueprint html-components rule

Previews: documentation | landing | table

@blueprint-bot
Copy link

add format-check CI task

Previews: documentation | landing | table

@blueprint-bot
Copy link

fix ts-jest configuration in eslint-plugin

Previews: documentation | landing | table

@adidahiya adidahiya marked this pull request as ready for review April 21, 2020 15:27
@blueprint-bot
Copy link

split eslint-config into multiple configs so it can run in eslint-plugin

Previews: documentation | landing | table

@blueprint-bot
Copy link

Merge remote-tracking branch 'origin/develop' into ad/fix-eslint-vscode

Previews: documentation | landing | table

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