prettier / prettier-vscode

Visual Studio Code extension for Prettier
https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode
MIT License
5.04k stars 446 forks source link

Add prettier-eslint integration #70

Closed kentcdodds closed 7 years ago

kentcdodds commented 7 years ago

The atom plugin has added this (maybe @robwise can give some tips). This way you and @RobinMalfait (who created prettier-eslint-vscode) could join forces to make this package even better!

RobinMalfait commented 7 years ago

Maybe it is an idea to join forces and also move prettier-vscode+prettier-eslint-vscode to the prettier namespace on GitHub?

CiGit commented 7 years ago

I've closed #14 lately. What's the advantage over running text -> prettier -> text -> eslint --fix -> text ?

This is already possible with 2 extensions (this one and Eslint) while saving:

{
  "editor.formatOnSave": true,
  "eslint.autoFixOnSave": true
}

Maybe luckily because the formatter runs before onsave events

It's also possible to setup a keybinding for those steps (not out of the box for now) Microsoft/vscode#871

Moving to the prettier org is tracked in #41 and prettier/prettier#781

@RobinMalfait won't mind having more contributors ;-)

kentcdodds commented 7 years ago

I don't know how things are handled in VSCode, but I know in Atom if you tried to use Prettier and the ESLint plugin together you'd get a pretty bad experience:

f6c19130-d8ba-11e6-9538-b4b4a788f63b

Also, I'm certain that it doesn't perform nearly as well.

CiGit commented 7 years ago

In code you also have those blinking change due to the intermediate changes being printed. That's the only bad thing I noticed. It may be a vscode bug/improvement.

I've seen something really nice in your plugin: you infer prettier's settings from eslint's one avoiding duplication and having config in your workspace. :+1:

We will have to think about how we handle settings precedence in our case.

kentcdodds commented 7 years ago

Perhaps @robwise can give some tips in that regard :+1:

Thanks for taking this under consideration @CiGit :)

robwise commented 7 years ago

Let me know if you have a speciifc question about what we're doing in prettier-atom. Currently, users can either choose to enable the eslint integration (i.e., use prettier-eslint under the hood) or not (i.e., use regular prettier). In the former case, we basically ignore every setting you've set for prettier because we let prettier-eslint pick it up entirely from the eslint configuration.

CiGit commented 7 years ago

@robwise Thanks for the info. What's happening if there is no configuration available for eslint? IMHO it should then fallback to prettier's provided settings but it seems it's just the opposite of what @kentcdodds' prettier-eslint is doing.

kentcdodds commented 7 years ago

it seems it's just the opposite of what @kentcdodds' prettier-eslint is doing.

🤔 I'm not sure how you're getting that. It clearly states:

Defaults if you have all of the relevant ESLint rules disabled (or have ESLint disabled entirely via / eslint-disable / then prettier options will fall back to the prettier defaults

CiGit commented 7 years ago

Sorry for the confusion. I was meaning user's provided prettier configuration which in prettier-atom is dropped.

  1. Use eslint as source of truth
  2. Fallback to user's config for missing settings
  3. Fallback to prettier's default for the rest.
kentcdodds commented 7 years ago

Ah, gotcha. Yeah, with prettier-atom, we try to make it very simple: If you check the eslint integration button, then none of your other settings matter. That makes things pretty clear. But I suppose it could also make sense if you wanted to fallback to settings in the IDE if eslint is not configured...

robwise commented 7 years ago

@kentcdodds in fact, some users have requested this. I think it would be okay to do it, although yes, it would be a bit more complicated

kentcdodds commented 7 years ago

Come to think of it, now that prettier supports no semicolons, I would also like this! 😅

robwise commented 7 years ago

@kentcdodds If we somehow allow passing the fallback prettier options to prettier-eslint when invoking it, then I can hook this up pretty easily in prettier-atom.

kentcdodds commented 7 years ago

Yep, that's possible 😀 https://github.com/prettier/prettier-eslint/blob/master/README.md#prettieroptions-object

robwise commented 7 years ago

I guess we'd do the same thing but call it fallbackPrettierOptions or something?

CiGit commented 7 years ago

Great! https://github.com/prettier/prettier-eslint/pull/73 👍

Question to prettier-eslint users: Does it make sens to use it without having it as a project dependency ? $HOME/.eslintrc If not we could simply pick it up from the project's dependencies and use it, thus avoiding an option.

kentcdodds commented 7 years ago

I could have a project with eslint configured, but not prettier-eslint. I would prefer to have it be: default to locally installed version, if it doesn't exist, use the plugin's version. Does that make sense?

CiGit commented 7 years ago

Definitely. So I won't remove the option

Does it make sens to load prettier-eslint from the project? If I understand it correctly prettier-eslint will load eslint/prettier from the project if I give it a filePath But do you expect some output changes between prettier-eslint's different versions? Therefor project's version may differ from bundled one?

kentcdodds commented 7 years ago

Yeah, I don't expect the output too differ. It's probably fine to just use the plugin's version.

CiGit commented 7 years ago

Thanks for your time. Will update my PR accordingly and wait for a review (there are a lot of changes involved)

By the way, nice work on your eslint-prettier, I've played around with it in my vscode integration and it's well better than prettier -> eslint --fix 😉

kentcdodds commented 7 years ago

Wonderful! Debugging tip: open up Dev tools and set: process.env.LOG_LEVEL = 'trace' Then run the format command and you'll get a bunch of useful logs from prettier-eslint 😀

CiGit commented 7 years ago

a bunch .... Thanks for the tip. It also made me correct my definitions 😆

diff --git a/src/types.d.ts b/src/types.d.ts
index f8ef5b9..d0a4ab0 100644
--- a/src/types.d.ts
+++ b/src/types.d.ts
@@ -71,7 +71,7 @@ interface PrettierEslintOptions {
     /**
      * The level for the logs
      */
-    loglevel?: LogLevel;
+    logLevel?: LogLevel;
     /**
      * Run Prettier Last. Default false
      */
kentcdodds commented 7 years ago

Thanks so much for doing this. It's great to converge solutions :)

github-actions[bot] commented 4 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.