prettier / eslint-config-prettier

Turns off all rules that are unnecessary or might conflict with Prettier.
MIT License
5.46k stars 255 forks source link

Why not combine "prettier/*" configs to one? #173

Closed thorn0 closed 3 years ago

thorn0 commented 3 years ago

Add extra exclusions for the plugins you use like so: extends: ..., "prettier/@typescript-eslint", "prettier/babel" ...

I think this step unnecessary complicates things for beginners. Why simply not have one big config prettier containing all those rules from prettier/* set to off. I was sure this was done out of technical necessity. I thought ESLint would error on unknown rules if you use prettier/react without the react plugin, but turns out it's not the case. ESLint (I tested 6.x and 7.x) is perfectly okay with unknown rules in the config if they're set to off. So why the complexity? (Truly sorry if already discussed.)

lydell commented 3 years ago

The reason was because ESLint used to fail for unknown rules, even when set to off.

Or so I thought. Now I’m not able to reproduce.

I wanted to have one single config from the start. If that had worked all the time then this is a big failure … lots of people forget to extend all the prettier/* configs they need.

We should investigate this a little bit more and then combine them up, for sure.

lydell commented 3 years ago

Fixed in v8.0.0.

iva2k commented 3 years ago

That's wonderful!

However, removal of prettier/@typescript-eslint etc. is a breaking change, affecting e.g. CI/CD pipelines, and it should be at least mentioned in the docs.

There are many projects that follow various how-to's and many of them broke overnight, especially the ones that pulled eslint-config-prettier as a dependency and did not have its version pinned down.

Even further, a better way would have been to keep old configs for a while and show 'deprecated' warning, allowing maintainers to fix the problem.

lydell commented 3 years ago

@iva2k Yeah, I just got the same feedback on Twitter. I made it a little bit better in 8.1.0 at least:

❯ npx eslint index.js

Oops! Something went wrong! :(

ESLint: 7.20.0

Error: Cannot read config file: /Users/lydell/stuff/eslint/node_modules/eslint-config-prettier/@typescript-eslint.js
Error: "prettier/@typescript-eslint" has been merged into "prettier" in eslint-config-prettier 8.0.0. See: https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21

affecting e.g. CI/CD pipelines

It is? How? CI pipelines should install versions pinned by package-lock.json or yarn.lock. Or at the very least be locked to known major versions. Otherwise the CI will be breaking all the time.

and it should be at least mentioned in the docs.

It is here: https://github.com/prettier/eslint-config-prettier#installation

ℹ️ Note: You might find guides on the Internet saying you should also extend stuff like "prettier/react". Since version 8.0.0 of eslint-config-prettier, all you need to extend is "prettier"! That includes all plugins.

iva2k commented 3 years ago

Thanks for quick response. The 8.1.0 is improving things. And that's all awesome work that is used by so many! I only want to suggest better docs.

My point was that current docs don't mention old way of doing things and that it broke. Current docs are fine if you start from scratch, but existing projects that were done some time ago one way or another suddenly stopped working. Googling for the error returned nothing (except the old how-to's that mention old configs). Would be nice if the README said something like "Breaking change: before v8 configs were split into multiple files . Now these files should not be used as it will cause errors."

In my particular case I did not have CI broken, as I was improving prettier setup and I had local build fail suddenly with mysterious message. I lock major/minor, and of course I have package-lock.json. When cleaning stuff up I noticed there was no dependency on eslint-config-prettier in package.json, as it was pulled in by eslint-plugin-prettier, but it was used in project configs. After I added the missing dependency of eslint-config-prettier to package.json, it broke without any other changes (I used typical npm i -D eslint-config-prettier which pulled latest v8 that was just released). Finding this merge was not trivial to be able to fix config files to resume passing CI. So my observation is that docs don't tell enough to figure out this stuff.

thorn0 commented 3 years ago

@iva2k

current docs don't mention docs don't tell enough

What docs did you look at? Both README and CHANGELOG include this info.

iva2k commented 3 years ago

Apparently google had not indexed the recent changes at the time, so searching for "prettier/@typescript-eslint" error did not find the latest changelog or anything for the error. Also these config names are not found in README, so the very helpful note is not showing on the radar.

Now it should be better, given that v8.1.0 more clearly states the error.

satanTime commented 3 years ago

Hi everybody,

somehow I'm missing something. There is a PR to v8, but it fails: https://github.com/satanTime/nx-nestjs-angular-graphql/pull/60 https://github.com/satanTime/nx-nestjs-angular-graphql/blob/renovate/root/eslint-config-prettier-8.x/.eslintrc.json

Cannot read config file: /root/project/node_modules/eslint-config-prettier/@typescript-eslint.js
Error: "prettier/@typescript-eslint" has been merged into "prettier" in eslint-config-prettier 8.0.0. See: https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21
Referenced from: /root/project/node_modules/@nrwl/eslint-plugin-nx/src/index.js
npm ERR! code ELIFECYCLE

I checked the link, and cannot get the problem. I have only plugin:prettier/recommended and assume that it should be enough, but it is not.

I hope that you know where the issue is.

Thank you in advance!

lydell commented 3 years ago

@satanTime The configs you extend can in turn extend other configs. The error message you posted mentions “@nrwl/eslint-plugin-nx” – I assume you use that? If so, there’s a PR that fixes the issue: https://github.com/nrwl/nx/pull/4899. I’m not sure if they have released that to npm yet.

satanTime commented 3 years ago

yep, you are right! Thanks for the help!