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

lint: prettier vs eslint #22

Open
casperdcl opened this issue May 13, 2021 · 13 comments
Open

lint: prettier vs eslint #22

casperdcl opened this issue May 13, 2021 · 13 comments
Labels
dependencies Pull requests that update a dependency file help wanted Extra attention is needed

Comments

@casperdcl
Copy link
Contributor

casperdcl commented May 13, 2021

This affects all JS repos.

Considerations:

Issues:

Original discussion: #18 (comment)

CC @iterative/studio @iterative/cml @rogermparent @julieg18

@casperdcl casperdcl added dependencies Pull requests that update a dependency file help wanted Extra attention is needed labels May 13, 2021
@rogermparent
Copy link

rogermparent commented May 13, 2021

  • prettier is supported everywhere, can lint *.js, and thus could replace eslint completely. However are there any advantages of continuing to use eslint?

Prettier is specifically built only to handle code style issues, while ESlint can do code-quality issues. We'll need both, but I agree that we should minimize the duplication between ESlint and Prettier.

We use this package or something similar in many of our js repos, which effectively runs prettier as an ESLint step. This is a good option as far as DX because all of a file's lint issues, style or code quality, are reported in one program.
We also have the simpler option of always using prettier for style issues and only run eslint for the rules and files prettier doesn't cover- this would obviously be easier to implement, but lacks a bit in DX because a file with both style and code quality issues will have its report split across two lint applications.

Issues:

This eslint config doesn't have prettier rules, so this is intended- it's an example of the second implementation type. We can certainly introduce a prettier-in-eslint plugin to give each file one program.

I believe it's run on Gatsby build with gatsby-plugin-eslint, though I ended up leaving that project before implementing the Husky/lint-staged setup and npm scripts we usually run ESLint with so we'll eventually want to address that for this site anyway.

@julieg18
Copy link

Prettier is specifically built only to handle code style issues, while ESlint can do code-quality issues. We'll need both, but I agree that we should minimize the duplication between ESlint and Prettier.

Agreed @rogermparent! Prettier and eslint are used for different things.

This eslint config doesn't have prettier rules, so this is intended- it's an example of the second implementation type. We can certainly introduce a prettier-in-eslint plugin to give each file one program.

Using a plugin that gives each file one program makes sense as well! I can open up an issue for Iterative.ai if needed.

@casperdcl
Copy link
Contributor Author

casperdcl commented May 14, 2021

Hmm I see. There are similar problems with Python (multiple linters which overwrite files with different specialisms) where config to prevent conflicts can be a pain.

@rogermparent
Copy link

rogermparent commented May 14, 2021

While that repo's lint config has many of the standard features we do for all project linting, Studio is considered the best practice for linting within our current projects because it had a lot of work put in it from outside help to tune it to balance requirements and DX a bit closer toward DX than most defaults. We recently adopted the studio lint rules for the VSCode project and only have needed minimal modifications.

The primary difference between iterative.ai and studio's lint is that studio actually uses a prettier-in-eslint plugin- also, I'm sure the specific rules are very different between these two.

  • what would you put into .restyled.yaml (or would you use a different automation solution)?

I'm not too familiar with restyled and haven't set it up yet, only interacted with the pre-existing setup on dvc.org. The workflow I've gotten familiar with here uses Husky to lint on the developer's end before commit and then a standard GH Actions CI prettier/eslint run makes sure PRs are formatted and errors if unlinted code somehow made it through because of --no-verify or a broken Husky setup.

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented May 14, 2021

@rogermparent

I'm not too familiar with restyled and haven't set it up yet

Interesting... I did not setup it also but I thought that restyled worked in all our repos. I. had to change CML's config to match restyled

The workflow I've gotten familiar with here uses Husky to lint on the developer's end before commit and then a standard GH Actions CI prettier/eslint run makes sure PRs are formatted and errors if unlinted code somehow made it through because of --no-verify or a broken Husky setup.

Thats what we do to also, here is also building dist version

@mvshmakov
Copy link

As was correctly stated by @rogermparent and @julieg18, these two packages are used for different purposes.

The ESLint exposes parsed AST to external plugins that are capable of various things. They can detect obvious security issues (e.g., regular expressions), validate JSX accessibility on the fly (without transpiling the code), prevent incorrect async operations, find code smells, or limit cognitive complexity, etc.

The Prettier is used just to lint the code according to the set of predefined rules extendable by external configs. It just makes it convenient to format-on-save and improves readability.

There are alternatives, for instance, great https://rome.tools/, but you can barely use them due to the early stage of development, unfortunately.

In my opinion, both of discussed tools are required to set up in modern web development. The question here is which plugins to use. As can see from the project source code, it is just about Node.JS, and no complex markdown/frameworks are involved, so the setup would be different from what we have in the Studio/main website. It should be more close to what is present in the CML, but the Studio still has some of the Node.JS-related plugins that are worth checking out.

@casperdcl
Copy link
Contributor Author

casperdcl commented Jun 3, 2022

Hello from 1 year in the future!
Should we have template repos? Or at least a brief list of things to set up?

  • Sounds like there are two different approaches: lib (Studio, VSCode) and website (iterative.ai, dvc.org)
  • config file contents to commit
  • GH integrations to set up

(PS /CC @dacbd about restyled etc.)

@dacbd
Copy link
Contributor

dacbd commented Jun 3, 2022

I have nothing against restyled so long as it doesn't open PRs, https://github.com/github/super-linter

@shcheklein
Copy link
Member

For all docs / websites (especially blog) we want to keep restyled. Less important for TS/JS there, but MD should stay.

@mvshmakov
Copy link

Should we have template repos? Or at least a brief list of things to set up?

We have a good enough brief list of things to setup - https://www.notion.so/iterative/Project-Guidelines-c9625673da8d4cb692b5989ec396cf61.

On the notion of template repos, I suggest us first answering the question what do we really want rn: to have template repos, or shared company-wide code style packages?

I personally feel like shared packages are potentially more beneficial for us (e.g., easier to switch/help other projects cross-company), but will be happy to help with setting up a template repo as well.

@casperdcl
Copy link
Contributor Author

Shared package also sounds interesting... presumably it covers less than a template repo (vis. e.g. https://github.com/iterative/py-template), but is maybe good enough?

@mvshmakov
Copy link

mvshmakov commented Jun 6, 2022

These two things serve a bit different purposes to my mind. Shared packages will stay in sync across projects. We can change anything in them, bump an NPM version and dependent projects will just pull a new version from registry on the next packages update. Changes in the template repo most likely will affect only newly generated repos as nobody will care to implement/transfer them in projects.

In the context of this specific issue, we should probably think about introducing shared packages for prettier/eslint/restyled. Though if we have a particular use case in mind where we need to have a quick-to-bootstrap project template at this point of time (e.g., somebody wants to start a new JS-related project from scratch), let's move with this option first. Any thoughts?

Btw, I don't think everybody knows about this issue, so posted it in #javascript.

@dacbd
Copy link
Contributor

dacbd commented Jun 6, 2022

worth a look? https://github.com/npm/template-oss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants