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

Bolt tslint to eslint #529

Merged
merged 19 commits into from
Aug 5, 2020
Merged

Conversation

kian2attari
Copy link
Contributor

@kian2attari kian2attari commented Jun 26, 2020

Summary

This PR aims to migrate Bolt.js from TSLint to ESLint. I used the typescript-eslint plugin to allow ESLint to work with TypeScript. I decided to go with this plugin as it was supported by both the backers behind TSLint (Palantir), who deprecated TSLint in favor of typescript-eslint, and the actual TypeScript team themselves.

Details

In the process of migrating Bolt to ESLint, I used the tslint-to-eslint-config project (as recommended by typescript-eslint) to automate the initial bulkwork of the process such as converting all the TSLint rule flag comments in all files to ESLint-compatible comments.

To match the original TSLint configuration as closely as possible (which extended the tslint-airbnb-config), I extended the eslint-config-airbnb-typescript plugin in the .eslintrc.js file. I also included recommended plugins like @typescript-eslint/recommended-requiring-type-checking to match the TSLint behavior. I commented out/disabled some of the recommended ESLint plugins as they introduce new rules that the project currently does not follow, raising linting errors. One challenge with this migration is that ESLint is more strict than TSLint. Since the new rules would require some changes in Bolt's source code (many of which can be fixed automatically), I'd like to discuss potentially including them as they do seem valuable. Alongside these potential additions, I also added some JSDoc rules that are currently commented out.

I'd also like to discuss the idea of using Prettier for code formatting (ex. checking indentation) rather than the linter as recommended by the typescript-eslint and tslint-to-eslint-config team. Certain TSLint rules, such as "whitespace", are supposed to be replaced by Prettier when migrating to ESLint (as recommended in the roadmap) since there is no ESLint equivalent. Prettier also does a much better job at checking formatting than ESLint with regards to checking indentation (relevant issue here). The indent rule raises quite a few arbitrary errors which is why I decided to comment it out for this PR.

I also added src/middleware/builtin.ts to the .eslintignore file because it was the only file that was raising linting errors. Its formatting seems very different from the other files in the project and the errors are almost all formatting issues (ex: line length of 122 instead of 120, missing return types on functions, missing line breaks, not using dot notation etc). I think this is worth discussing

Requirements (place an x in each [ ])

@kian2attari kian2attari requested a review from aoberoi June 26, 2020 21:52
@CLAassistant
Copy link

CLAassistant commented Jun 26, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #529 into main will increase coverage by 0.11%.
The diff coverage is 83.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
+ Coverage   83.13%   83.24%   +0.11%     
==========================================
  Files           7        7              
  Lines         593      597       +4     
  Branches      184      193       +9     
==========================================
+ Hits          493      497       +4     
+ Misses         68       67       -1     
- Partials       32       33       +1     
Impacted Files Coverage Δ
src/conversation-store.ts 100.00% <ø> (ø)
src/middleware/process.ts 100.00% <ø> (ø)
src/ExpressReceiver.ts 64.92% <71.42%> (-0.30%) ⬇️
src/App.ts 87.81% <80.59%> (-0.02%) ⬇️
src/errors.ts 100.00% <100.00%> (ø)
src/helpers.ts 90.90% <100.00%> (ø)
src/middleware/builtin.ts 82.78% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be12cd9...0fbeb71. Read the comment docs.

@seratch seratch requested a review from stevengill June 29, 2020 00:49
@seratch seratch added the tests M-T: Testing work only label Jun 29, 2020
package.json Outdated Show resolved Hide resolved
@kian2attari kian2attari closed this Jul 8, 2020
@kian2attari kian2attari reopened this Jul 8, 2020
@kian2attari kian2attari self-assigned this Jul 8, 2020
@clavin clavin changed the base branch from master to main July 8, 2020 03:09
@kian2attari
Copy link
Contributor Author

kian2attari commented Jul 8, 2020

Summary of new changes (07/08/2020)

Added Prettier to the linting process and used the recommended prettier/@typescript-eslint plugin to disable ESLint rules that conflict with Prettier. Also made some pretty major improvements to the ESLint configuration structure to make it more maintainable and readable.

Details

  1. Incorporated Prettier into the linting process, included a basic prettier config, and disabled ESLint rules that conflict with Prettier using prettier/@typescript-eslint
  2. Removed the enormous bunch of unnecessary rule specifications I had in the previous version of the .eslintrc.js. The eslintrc.js (which lints all files except spec/test files) and .eslintrc.test.js files now extend from a base ESLint configuration found in the eslint-config-base folder. The rules property in .eslintrc.js and .eslintrc.test.js respectively is now only used to disable a couple of the strict new rules (ex. from airbnb's TypeScript ESLint config) that the project currently does not follow. Most of these are easy/automated fixes so I think we should conform to them.
  3. Changed the .eslintignore to only ignore the node_modules directory as that's more typical convention. I was previously also globally ignoring test/spec files (ex *.spec.ts) here so that the regular .eslintrc.js would not lint them, while using the --no-ignore flag for the test-lint command so that the test files could be linted there. Instead of global ignores, I'm now locally ignoring the test files using the ignorePatterns property in .eslintrc.js.
  4. I updated some of the old TSLint line-specific rule disable comments to their ESLint counterparts and also added a few ones in new places since ESLint is so picky. I made sure not to change any of the actual code in the project files.
  5. Modified some of the ESLint rule configurations to match the rules of the old tslint configuration we had even more closely
  6. Updated the linting commands in package.json so be more straightforward and conventional to ESLint. Rather than specifying a glob like src/**/*/*.ts like we did with the TSLint commands, I specify the file extension(s) and the directory that should be linted.
  7. I removed a couple of now unused linting-related packages and deleted tslint.json 🧹

Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM. I'm fine with moving forward with this

.eslintrc.js Outdated
of action is likely to adopt these rules and make the quick (and mostly automated) fixes
needed in the repo to conform to these. ESLint and the airbnb-typecript config is more strict
than the original TSLint configuration that this project had. */
'import/first': ['off'],
Copy link
Member

Choose a reason for hiding this comment

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

Definitely might want to conform to some of these airbnb rules.

@stevengill
Copy link
Member

I did a few updates to this branch.

  1. I've setup prettier to actually run on save in vscode and added it to the npm run lint command.
  2. I removed some of the rules in .eslintrc.js that were put in place to work with our existing linting but are different than the standard airbnb typescript lint rules. I kept a few as it wasn't worth trying to refactor the code at the moment to make them pass. This means this PR now includes some style changes in our code. Take a look and decide if the changes that have landed are acceptable. They were all pretty minor to me.

@stevengill stevengill merged commit f8c25ff into slackapi:main Aug 5, 2020
@stevengill stevengill mentioned this pull request Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests M-T: Testing work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants