search

grammarly / focal

Program user interfaces the FRP way.
Apache License 2.0
723 stars 34 forks source link

Switch from TSLint to ESLint #74

Closed gabebw closed 3 years ago

gabebw commented 4 years ago

The bulk of this work was done automatically by npx tslint-to-eslint-config according to the instructions here.

I then manually compared the ESLint rules to the TSLint rules. Then I ran ESLint over the existing files and fixed rules that did not error but should (false negative).

I also copied lines that were ignored by a TSLint comment to also be ignored by an ESLint comment.

Each commit here is self-contained, so I recommend going through each commit one by one to see the process. The commits deal with, in rough order from oldest to newest:

Because ESLint is not recursive in the same way TSLint is, and does not typecheck, each package has its own yarn lint task that runs ESLint and the typescript compiler (for typechecking).

To run all linting tasks for all packages, run yarn lint in the root directory.

blacktaxi commented 3 years ago

Hey @oleksiilevzhynskyi, thanks for keeping this up to date!

I noticed that the no-redeclare rule seems to trigger a lot in this codebase, since we use the type + namespace merging pattern a lot. I tried to use a TS-specific alternative (@typescript-eslint/no-redeclare), but it actually intentionally flags such use case.

I see this as a useful lint rule overall, but also don't really agree on the default behavior. Given how both no-redeclare and @typescript-eslint/no-redeclare basically give the same result in our use case, it may seem that there's no difference but I think it's probably still better to use the TS-specific version.

Thoughts?

oleksiilevzhynskyi commented 3 years ago

@blacktaxi, thanks for pointing this out. @typescript-eslint/no-redeclare complaining only about the "merge" declaration of Atom. Another case it treats as correct usage. Changed.

blacktaxi commented 3 years ago

Thanks! Everything is green, but I checked the CI log and it seems that eslint doesn't work as expected on the tests and examples code (see here):

Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/index.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/app.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/add-input/index.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/animated-counter/index.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/big-table/index.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/bmi/index.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/change-color/index.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /home/runner/work/focal/focal/packages/examples/all/src/checkbox-undo-redo/index.tsx but found no valid, enabled rules for this file type and file path in the resolved configuration.
...

When I run this locally, I actually get a bunch of lint errors in these files (basically every time AppState is declared).

oleksiilevzhynskyi commented 3 years ago

Migrated to eslint-webpack-plugin from ts-loader.

tslint-loader was deprecated in favor of eslint-loader; eslint-loader was deprecated in favor of eslint-webpack-plugin

oleksiilevzhynskyi commented 3 years ago

@gabebw thank you for MR. eslint is finally merged! 🎉

gabebw commented 3 years ago

Thank you for getting it merged!

Sent from my iPhone

On Apr 2, 2021, at 8:34 AM, Oleksii Levzhynskyi @.***> wrote:

 @gabebw thank you for MR. eslint is finally merged! 🎉

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.