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

Prettier should not be included by default in eslint-config #556

Open
arthurgeron-work opened this issue Dec 5, 2022 · 3 comments
Open

Comments

@arthurgeron-work
Copy link

arthurgeron-work commented Dec 5, 2022

Introduction

Prettier is know to cause many ESLint issues & conflicts as well as being slow to run since it has to "boot up" prettier executable each time it parses a file, hence it should not be included by default on @react-native-community/eslint-config, I believe it's better to leave it to developers to decide wether to include it or not.

Details

ESLint on a relatively small file with prettier:

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
prettier/prettier                    |   442.890 |    85.4%
react/no-multi-comp                  |    10.953 |     2.1%
jest/no-identical-title              |     8.454 |     1.6%
no-cond-assign                       |     7.448 |     1.4%
react/no-unstable-nested-components  |     5.258 |     1.0%
jest/no-disabled-tests               |     3.456 |     0.7%
no-restricted-imports                |     3.415 |     0.7%
react-hooks/rules-of-hooks           |     3.233 |     0.6%
@typescript-eslint/naming-convention |     2.943 |     0.6%
no-new-func                          |     2.362 |     0.5%

ESLint on a relatively small file without prettier:

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
react/no-multi-comp                  |     8.143 |    13.8%
indent                               |     6.473 |    11.0%
jest/no-identical-title              |     4.776 |     8.1%
@typescript-eslint/indent            |     4.146 |     7.0%
jest/no-disabled-tests               |     3.971 |     6.7%
@typescript-eslint/naming-convention |     2.659 |     4.5%
react/no-unstable-nested-components  |     2.505 |     4.2%
react-hooks/exhaustive-deps          |     1.536 |     2.6%
react/require-render-return          |     1.039 |     1.8%
import-helpers/order-imports         |     1.006 |     1.7%

As it can be seen prettier itself is ~55x slower than the heaviest of our rules, that delay scales up the more files we get and slows ESLint job in general.

Discussion points

  • Remove prettier/prettier from "extends" in eslint-config
@NickGerleman
Copy link

NickGerleman commented Dec 5, 2022

Prettier is know to cause many ESLint issues & conflicts

Could you elaborate on this more? The config is set up to use prettier as a peerDependency, so that the user controls the version. It also sets style options in a prettierrc so that raw prettier should create results identical to ESLint.

I think prettier provides a lot of value in the config. Almost every JavaScript project I've used pulls it in for formatting, and it can be very useful to check formatting during linting. I'm not sure how many projects see real issues caused by linting performance, and anecdotally, sets of rules like @typescript-eslint/reccomended (which RN doesn't currently extend) can be expensive in similar ways. We would also ideally measure performance in a less synthetic context. E.g. a one-time cost could show much more in a single file test, compared to the performance in a more realistic scenario.

It is also still possible to use ESLint for a react-native project without extending @react-native-community/eslint-config, and to still use all of the underlying plugins. Trivially, one could copy/paste the config into their own ESLint config and modify it (or change settings when extending). So I think we should be aiming for a (necessarily opinionated) good default experience, knowing that users can ultimately choose something different.

@arthurgeron-work
Copy link
Author

arthurgeron-work commented Dec 15, 2022

Prettier is know to cause many ESLint issues & conflicts
Could you elaborate on this more? The config is set up to use prettier as a peerDependency, so that the user controls the version. It also sets style options in a prettierrc so that raw prettier should create results identical to ESLint.

My point is exactly that, there are rules to replicate prettier behaviour without any of the performance compromises. Prettier is a parser/executable and needs to start up each time it's going to run, it cannot be run as a daemon, so for each tab in VSCode that is going to run a lint job it needs to bootup again, I had my VSCode's "Quick Fixes" quickly start to hang on "Running Save Participants..." because of that, it took me some debugging and time to find the culprit; This problem also extends to pre/post commit hooks, CI/CD, etc.

That said there are some solutions for running prettier as a daemon (e.g. prettierd) but don't support all prettier features and requires a global install or brew setup in the machine alongside cat_d, which not only isn't compatible with eslint but also greatly increase the work necessary to run projects as compared to a C.I.E. (clone, install and execute) project. Ultimately it's a overkill just to run lint on files.

I think prettier provides a lot of value in the config. Almost every JavaScript project I've used pulls it in for formatting, and it can be very useful to check formatting during linting. I'm not sure how many projects see real issues caused by linting performance, and anecdotally, sets of rules like @typescript-eslint/recommended (which RN doesn't currently extend) can be expensive in similar ways. We would also ideally measure performance in a less synthetic context. E.g. a one-time cost could show much more in a single file test, compared to the performance in a more realistic scenario.

I've also been using/seen prettier in basically all react/react-native project, I don't think that implies it's necessarily an argument that it should keep being used, I'm pretty sure most maintainers and developers are also not aware of the huge impact that prettier has on the time to complete a lint call. Being widely used does not mean there aren't better alternatives out there or that the current state of things shouldn't be improved.

Like I mentioned before and as far as I've researched most of prettier's rules can be replicated with alternative eslint rules except for Prose Wrap. So RN can be still opinionated, it just doesn't need to slow down the lint executions.

Furthermore I use @typescript-eslint in my projects and haven't seen any rule take more than 10ms to execute, which is meaningless compared with prettier/prettier taking ~55x to run.

It is also still possible to use ESLint for a react-native project without extending @react-native-community/eslint-config, and to still use all of the underlying plugins. Trivially, one could copy/paste the config into their own ESLint config and modify it (or change settings when extending). So I think we should be aiming for a (necessarily opinionated) good default experience, knowing that users can ultimately choose something different.

Being as big as RN is the recommended configuration will be the default for most projects, so all unnecessary performance compromises should be avoided as those will potentially be extended to a whole community, specially if they can be completely avoided with equivalent alternatives.

With all that said you're also free to test this in bigger projects, running it with TIMING=1 before the lint command prints all the timing data. If there's any specific realistic scenario you have in mind let me know and I'll run benchmark tests on it.

@arthurgeron-work
Copy link
Author

When migrating to newer version of this package I faced a error in the eslint because my project does not use prettier.
I'd like to make my argument again, doesn't react-native-community eslint config intend to bring the best practices?
Prettier is a lib focused on code style/format preferences and a strongly opinionated one at that, I don't think extending this lib should not impose styles/solutions for styling, even just adding prettier there forces you to have it installed even if you don't want to use it.

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

No branches or pull requests

2 participants