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: Upgrade ESLint and all plugis & revalidate the config and ALL of the rules #15287

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

peter-sanderson
Copy link
Contributor

@peter-sanderson peter-sanderson commented Nov 6, 2024

This is PR that upgrades the ESLint and all its plugins + does refactoring of the whole configuration.

  • Configuration is now written in modern flat format.
  • Configuration is separated in @trezor/eslint but its used globally.
  • Some packages, if they need to define custom rules will require it in package.json and then they can define their own config.
  • Most of the newly recommended rules were introduced in previous mini-PRs (see the list here
  • I had to remove no-shadow rule, as it start reporting insane amount of stuff and it was hard to configure. I think we can solve it separately in future PR.

I have written a bunch of comments in the PR itself to explain stuff. I hope it help you to do code-review.


Performance:

Outcome of the reset && find . -type f -name ".eslintcache" -delete && time TIMING=1 yarn lint:js

This PR:

Rule                              | Time (ms) | Relative
:---------------------------------|----------:|--------:
@typescript-eslint/no-unused-vars |  6444.161 |    18.2%
import/no-extraneous-dependencies |  4620.629 |    13.1%
import/namespace                  |  4135.507 |    11.7%
import/named                      |  3041.406 |     8.6%
import/order                      |  2395.140 |     6.8%
react/no-direct-mutation-state    |  2234.944 |     6.3%
import/no-duplicates              |  1993.402 |     5.6%
react/require-render-return       |   673.360 |     1.9%
padding-line-between-statements   |   370.092 |     1.0%
react/no-deprecated               |   361.447 |     1.0%
TIMING=1 yarn lint:js  96,91s user 4,91s system 152% cpu 1:06,78 total

develop

Rule                                     | Time (ms) | Relative
:----------------------------------------|----------:|--------:
import/order                             |  2306.349 |    21.9%
import/no-extraneous-dependencies        |  1772.432 |    16.9%
@typescript-eslint/no-unused-vars        |  1282.584 |    12.2%
import/no-duplicates                     |   409.478 |     3.9%
jest/no-standalone-expect                |   331.732 |     3.2%
react/jsx-no-target-blank                |   312.352 |     3.0%
mdx/remark                               |   295.082 |     2.8%
padding-line-between-statements          |   244.067 |     2.3%
@typescript-eslint/no-restricted-imports |   198.185 |     1.9%
react/no-render-return-value             |   187.968 |     1.8%
TIMING=1 yarn lint:js  44,93s user 1,74s system 172% cpu 27,112 total

I assume the much longer run is due to many more rules and files are being checked as I removed many of the offs we had and enabled many of the checks for most of the packages. Also I suppose with recommended settings some of the rules were added on top of what we have.

Copy link

github-actions bot commented Nov 6, 2024

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 10
  • More info

Learn more about 𝝠 Expo Github Action

@@ -0,0 +1,3 @@
import { eslint } from '@trezor/eslint';

export default [...eslint];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Repository top-level config is now lust a link into package that contains all of the configuration.

@@ -31,7 +31,7 @@
"type-check": "yarn nx run-many --target=type-check",
"type-check:force": "rimraf -rf -- **/libDev && yarn type-check",
"test:unit": "yarn nx run-many --target=test:unit",
"lint:js": "eslint . --cache --cache-strategy content --ignore-path .gitignore",
"lint:js": "eslint . --cache --cache-strategy content --flag unstable_config_lookup_from_file",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flag loads configuration which is closest to the checked file. This enables the overriding of the rules per package.

See: https://eslint.org/docs/latest/use/configure/configuration-files#experimental-configuration-file-resolution

@@ -14,18 +22,12 @@ module.exports = {
name: getAbsolutePath('@storybook/react-webpack5'),
options: {},
},
babel: async options => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested the build of storybook and it works.

@@ -14,18 +22,12 @@ module.exports = {
name: getAbsolutePath('@storybook/react-webpack5'),
options: {},
},
babel: async options => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested the build of story book and it works.

@@ -2,7 +2,6 @@ const { ...baseConfig } = require('../../jest.config.base');

module.exports = {
...baseConfig,
testEnvironment: 'node',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate key

@peter-sanderson peter-sanderson force-pushed the eslint-upgrade5 branch 4 times, most recently from 694c6c9 to fb7a053 Compare November 7, 2024 17:21
@@ -9,15 +9,13 @@ const Wrapper = styled.div`
flex-direction: column;
`;

// eslint-disable-next-line local-rules/no-override-ds-component
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When overwrite happens inside the @trezor/components it wont get reported. And now the ESLint complains about unused ignore. It is here multiple times.

@@ -7,7 +7,6 @@ const Floating = styled.div`
margin-top: -2rem;
`;

// eslint-disable-next-line local-rules/no-override-ds-component
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorrect Link is from 'next/link' and not from @trezor/components. Now ESLint correctly raised error that this is unused ignore.

@@ -86,7 +86,6 @@ const Step = styled.div<{ $isActive: boolean }>`
`}
`;

// eslint-disable-next-line local-rules/no-override-ds-component
Copy link
Contributor Author

@peter-sanderson peter-sanderson Nov 7, 2024

Choose a reason for hiding this comment

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

This is override inside of the @trezor/produce-components this wont be reported by the rule and therefore it is unused ignore.

@@ -1,3 +1,4 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import * as Codegen from '@sinclair/typebox-codegen/typescript';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martykan I am not sure how to solve this. Please take a look.

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.

1 participant