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

fix(prettier): use recommended rules to avoid eslint conflicts, revert #797 #827

Closed

Conversation

scscgit
Copy link
Contributor

@scscgit scscgit commented Jul 17, 2021

https://github.com/prettier/eslint-plugin-prettier#recommended-configuration

If you use the default prettier extension instead of the recommended plugin:prettier/recommended and use prettier together with eslint, it can cause issues, for example with the following code:

// have any variable a
[].forEach((b) => a = b)

if you run npm run lint:js -- --fix, you will get
error Arrow function should not return assignment no-return-assign
instead of the code being fixed to ;[].forEach((b) => (a = b)) as it's supposed to.

Prettier officially recommends this setup, so I believe it to be a better default & long-term choice for new Nuxt developers.


An example of a use-case:
https://github.com/quicktype/quicktype#generating-code-from-json-schema
generates code like typ.props.forEach((p: any) => map[p.json] = { key: p.js, typ: p.typ }); in function jsonToJSProps, increasing learning curve by forcing users to look for workarounds once they include a similar code generator to their project.

Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

Is this related to #826 ?

@scscgit
Copy link
Contributor Author

scscgit commented Jul 17, 2021

@clarkdo nah it's an unrelated issue.

(but I'd lie if I said that I didn't just mistakenly come to incorrect conclusions about eslint-plugin-vue including the eslint-plugin-prettier after accidentally installing it as a direct dependency instead of devDependency, and then having to debug all the combinations... 😅)

@scscgit
Copy link
Contributor Author

scscgit commented Aug 6, 2021

I just noticed that this PR reverts #797, where it's suggested that this is "generally not recommended by the Prettier official". However, even though it may not be recommended when using Prettier as a standalone tool, the eslint-config-prettier used by this plugin seems to be necessary as long as users also install eslint. Maybe we could disable it if the eslint isn't selected? In any case, the current behavior without the plugin is unstable.

prettier/eslint-config-prettier: "Turns off all rules that are unnecessary or might conflict with Prettier."

@scscgit scscgit changed the title fix(prettier): use recommended rules to avoid eslint conflicts fix(prettier): use recommended rules to avoid eslint conflicts, revert #797 Aug 6, 2021
@scscgit scscgit force-pushed the fix/prettier-recommended-configuration branch from 3ea2aa0 to 5f05d22 Compare August 23, 2021 15:39
@scscgit
Copy link
Contributor Author

scscgit commented Aug 23, 2021

Updated version to 3.4.1 (though I can't find this version on https://github.com/prettier/eslint-plugin-prettier/releases/) and resolved conflicts. By the way, should we also resolve conflicts if they are caused only by snapshot tests?

@WinterYukky
Copy link
Contributor

WinterYukky commented Aug 24, 2021

I apologize for any disturbance caused by the PR I created.
If you want to fix this problem from the CLI, you can use prettier -w . . Or set prettier as formatter in the editor settings.
VS Code example

{
    "editor.defaultFormatter": "esbenp.prettier-vscode", // insert this line
    "editor.formatOnSave": true, // optional
}

This is the quote from official prettier.

When searching for both Prettier and your linter on the Internet you’ll probably find more related projects. These are generally not recommended, but can be useful in certain circumstances.

First, we have plugins that let you run Prettier as if it was a linter rule:

  • eslint-plugin-prettier
  • tslint-plugin-prettier
  • stylelint-prettier

These plugins were especially useful when Prettier was new. By running Prettier inside your linters, you didn’t have to set up any new infrastructure and you could re-use your editor integrations for the linters. But these days you can run prettier --check . and most editors have Prettier support.
The downsides of those plugins are:

  • You end up with a lot of red squiggly lines in your editor, which gets annoying. Prettier is supposed to make you forget about formatting – and not be in your face about it!
  • They are slower than running Prettier directly.
  • They’re yet one layer of indirection where things may break.

Nuxt.js does recommends VS Code as an editor, and VS Code supports Prettier.
Therefore, I think that re-installing eslint-plugin-prettier is a disadvantage for most people.

If you want to use the script in packages.json, I will create a PR with the script adjusted like this.

{
  "name": "eslint-prettier-test",
  "version": "1.0.0",
  "private": true,
  "scripts": {
    "dev": "nuxt",
    "build": "nuxt build",
    "start": "nuxt start",
    "generate": "nuxt generate",
    "lint:js": "eslint --ext \".js,.vue\" --ignore-path .gitignore .",
    "prettier:js": "prettier --ignore-path .gitignore **.js **.vue", // insert this line
    "lint": "yarn prettier:js -c; yarn lint:js", // change this line
    "lint:fix": "yarn prettier:js -w; yarn lint:js --fix" // insert this line
  },
  "dependencies": {
    "core-js": "^3.15.1",
    "nuxt": "^2.15.7"
  },
  "devDependencies": {
    "@babel/eslint-parser": "^7.14.7",
    "@nuxtjs/eslint-config": "^6.0.1",
    "@nuxtjs/eslint-module": "^3.0.2",
    "eslint": "^7.29.0",
    "eslint-config-prettier": "^8.3.0",
    "eslint-plugin-nuxt": "^2.0.0",
    "eslint-plugin-vue": "^7.12.1",
    "prettier": "^2.3.2"
  }
}

@scscgit
Copy link
Contributor Author

scscgit commented Aug 25, 2021

Thanks for the clarification, we now have the context behind the decision: We've removed Prettier from ESLint which was previously able to apply Prettier directly, even as part of its own --fix parameter usage. (I think it'd be worth explicitly communicating this opinionated decision to end users of the generator, as we're currently quite silent about these matters.)

It seems you're right and this issue of eslint conflict can be resolved by just running prettier as part of lintfix before running eslint. I already have an open PR #829 where I suggest adding a lintfix script, so it may be a good idea to explicitly reorder the scripts. (Maybe this will also fix another consistency issue that I'm having where sometimes I have to run lintfix multiple times in a row. I haven't found any source of truth stating the "correct order", which I think would be worth documenting.)

Back to the topic of ESLint conflict, even though I'd prefer running only eslint on the generated code (like JSON schema parsers) without having to also run prettier (causing unnecessary code diff), it seems the same no-return-assign issue persists even if I don't include any prettier plugin in .eslintrc.js, so it's indeed not an issue of prettier. The correct usage seems to thus require adding && prettier --write generated.ts && eslint --fix generated.ts at the end of the code generator script. But just to cover the remaining scenarios, this question remains: if prettier always fixes these eslint rules, why do we need to keep them enabled? Wouldn't it be better for eslint-config-prettier (which we already use) to exclude them directly?

A note about your suggested "editor.formatOnSave": true, just for reference: there were some reasons (but I don't remember exactly why) for replacing it with the other alternative. However, these settings don't work at all if you already use auto-saving (which I'd say is objectively a better default), so we're stuck with using the formatter directly, or using it as part of lintfix. My VSCode configuration (saved in project as .vscode/settings.json) includes the following, requiring an exception for [vue]:

{
  "files.eol": "\n",
  "editor.rulers": [80],
  "editor.codeActionsOnSave": ["source.fixAll"],
  "editor.defaultFormatter": "esbenp.prettier-vscode",
  "[vue]": {
    "editor.defaultFormatter": "octref.vetur"
  }
}
  • I was thinking about also opening a PR with this and .vscode/extensions.json to automatically install a correct (compatible) setup (but it'd be easier if people shared multiple points of view about upsides & downsides of some of them).

(And of course, I don't agree that VSCode is the only de-facto IDE, as people often prefer superior products like IntelliJ IDEA rather than eating up all Microsoft's bad decisions. However, in case of Vue development I noticed IDEA having quite a lot of issues at the moment, including the inability to recognize & jump to components, reporting false-positive errors in templates, and also being unable to configure Prettier as a default formatter, having to be used manually with a different key-bind)

Based on nuxt/nuxt#9563 there seems to be a trend of preferring the usage of prettier via ESLint, so we should consider which default to go forward with. Some points to consider:

  • IDE compatibility and the performance hit of using multiple plugins, as an offset of the minor performance gain: https://stackoverflow.com/a/68880413/8816585
  • Can we properly configure the implicit Prettier (in Eslint), e.g. to exclude some files, and does it support all file types when run via ESlint? Doesn't this have different behavior in some situations? (I believed that I had to run both eslint and prettier, but now I'm starting to doubt whether I still really had to it since starting to use the plugin.)
  • Do all IDEs support Prettier, or can they be configured to use it via Eslint without requiring this to be installed on a project level?

@WinterYukky
Copy link
Contributor

WinterYukky commented Aug 26, 2021

Thanks for the reply.
The PR #829 you created is looks good. I'll take a look when I have time😃

About generated js. If you don't mind, can you tell me the procedure for generating the problematic js file? I don't know if I can help you, but I'll look into it.

I didn't know editor.formatOnSave setting is not working when autoSave is enable. As such, In some cases we need lintfix script.

I don't agree that VSCode is the only de-facto IDE me too. But installing eslint-plugin-prettier does not solve the IDE problem.  eslint-plugin-prettier solves this problem by running prettier during "eslint --fix", but lintfix script is the solution for editors that cannot run eslint.

This description by Prettier is also important.

You end up with a lot of red squiggly lines in your editor, which gets annoying. Prettier is supposed to make you forget about formatting – and not be in your face about it!

This is a trivial issue for those of us who are somewhat familiar with it. However, for beginners, it is one of the factors that make the hurdle higher. I often teach beginners how to develop with Nuxt.js and other development methods, and they struggle because they can't decide if the problem provided by eslint-plugin-prettier is a syntax problem or just a formatting problem.
Prettier is supposed to make you forget about formatting.

@scscgit
Copy link
Contributor Author

scscgit commented Aug 26, 2021

About the no-return-assign conflict, I think it's enough to just illustrate it on [].forEach((b) => a = b). But as I said, it seems to get fixed if we run prettier before eslint as part of the lintfix, and the IDE can also run prettier directly, so I agree that we should most likely close this PR and instead just fix the lintfix order (so that even users without IDE support are able to fix their code, though I have no idea how long it'll take for my lintfix to get merged here). For now I'm uninstalling the plugin:prettier/recommended from my project as prettier's docs seem convincing to me (it's funny how their own plugin's Readme says something else than their official guidelines), but we should first have others review what's better for the majority of projects.

  • (unrelated to the issue) And yeah, the proper IDE setup is far from trivial nowadays; you almost have to become a full-time expert specialized on configuring IDE and their workarounds, especially if you care about code quality at all, or about any other developers who can't read your mind and may have their own needs. It's just sad that this isn't made more transparent by Nuxt, as everyone is silent on these day-to-day matters that nobody is exempt from struggling with, and it takes months before any maintainer notices even some of the most obvious common sense connections. Learnability should be a virtue worth focusing on; just like VS Code keeps adding thousands of features without fixing basics things like "File history" or providing proper tab layouts that focus on productivity rather than forcing their own twisted sense of design that everyone must accept, Nuxt still also has ways to go about making decisions driving zero-config less of a black-box and properly covering the user's journey end-to-end. As an illustration, it could be discussed whether eslint-config-prettier "should" exclude the abovementioned rules that conflict with prettier, and if not, then to probably exclude them using this generator, rather than just hoping that over the next couple of years these libraries and IDEs will eventually fix their own defaults. The same applies to making opinionated decisions on how should one learn to make their own decisions about the libraries. When it comes to beginners, code style probably isn't their priority, but once they start to work on team projects (especially if they follow documentation rather than being guided by teachers), it's quite cruel not to set up auto-format capabilities for them, expecting them to extensively research nuances of prettier and eslint in order to have confidence when setting up any collaboration.

@scscgit scscgit marked this pull request as draft August 26, 2021 16:16
@kissu
Copy link

kissu commented Sep 5, 2021

@scscgit that's why I posted a how-to on stackoverflow to provide a straightforward configuration for newcomers.

@scscgit
Copy link
Contributor Author

scscgit commented Sep 6, 2021

@kissu Yeah thanks, we'll see what the maintainers of create-nuxt-app will have to say after considering both pros and cons of using Prettier via ESLint. (There has been a very low activity here over last few months, as I still have several pending PRs.) I'm keeping this PR disabled for now, as I haven't tested the ESLint behavior & compatibility myself except the matter of ESLint conflicts, so there may be additional changes needed. I won't be working on this, so feel free to join in the PR if you have updates. We could even consider something like updating this PR to add an optional setting like "use standalone prettier".

I personally believe that, at this moment, it's better for the default setup to use standalone Prettier based on its official recommendation, as I haven't observed any critical issues on the latest version. The IDEs that you've mentioned are all covered by https://prettier.io/docs/en/editors.html so if there are any specific cases that aren't covered, it'd be nice to explicitly communicate them, probably even linking them as a GitHub issue for Prettier. It's quite likely that some combinations (Prettier or ESLint) will break again in the future, and it feels like Prettier will only provide up-to-date first-class support if we follow their official setup. I think we should track the specific issues (including difference in performance of the plugin choices, though sometimes a dedicated plugin provides additional benefits - after all, they continuously update it). Btw. your example includes both 'plugin:prettier/recommended' and 'prettier' rules, which seems redundant (and is also a reason why this needs to be better analyzed to avoid incorrect combinations), and the You'll also benefit from having Vue/Nuxt specific ESlint rules also has to be confirmed, as I believe the Nuxt-specific rule is preinstalled as plugin:nuxt/recommended. That said, if there are any objective benefits, it'd be great to include this in generator at least as an optional setup; or even better, we could look for ways how to set up project using only Prettier in such a way that developers can locally override it to be used via ESLint, without having to see any "pending Git changes". (And I don't see why not to put some optional/troubleshooting steps into generated README as part of the project.) I believe developers should always have a final say in what IDE setup they want to use (which includes not overriding their ESLint behavior), whereas the plugins should be a source of truth for enforcing code quality (before pushing any changes).

  • And let's not forget about the disadvantages of ESLint approach, including the abovementioned red squiggly lines.

This choice will also impact my PR #829 where the prettier would probably disappear from lintfix, so I'd prefer to get it merged first, and then this PR could remove the entry if we decide to go this way (so it's clear that it was intentional).

  • In any case, my primary issue with create-nuxt-app right now is the lack of prettier inside lintfix script, along with the lintfix itself (and the same for lint, which currently doesn't allow enforcing code style via commitlint), but it works very well once I configured it (together with a correct combination of VSCode extensions, which took ages to research by trial and error).

scscgit added a commit to scscgit/create-nuxt-app that referenced this pull request Sep 6, 2021
@clarkdo clarkdo closed this in f3e61cd Nov 18, 2021
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.

4 participants