Change Request: Consider adopting prettier internally

[bmish]

ESLint version

N/A

What problem do you want to solve?

Forgive me / feel free to close this issue if the answer to this inquiry hasn't changed. I couldn't find a dedicated issue for this discussion so I'm opening one here.

Obviously, prettier is popular in the ecosystem for automatic code formatting.

To date, we have not adopted prettier internally in the ESLint codebase because we want to dogfood our own formatting rules (link to comment):

As a side note, I'd love to see our code formatted with Prettier as well, but there's likely a discussion on GitHub somewhere explaining why we haven't adopted it.

Dogfooding. It's a lot easier to find bugs in formatting rules if we use them exclusively.

However, I'm raising this issue due to some recent developments:

1. We're on the verge of formatting our markdown docs with prettier: https://github.com/eslint/eslint/issues/17504
2. We recently deprecated formatting rules: https://github.com/eslint/eslint/issues/17522
3. Formatting/stylistic rules have been frozen since 2020: https://eslint.org/blog/2020/05/changes-to-rules-policies/

What do you think is the correct solution?

These developments could be an opening for us to adopt prettier internally in the ESLint codebase for JavaScript files, as it may be less important to dogfood our formatting rules when they are deprecated and frozen (as well as unit-tested like all rules) and we've already adopted prettier elsewhere.

If now is not the time, then would we consider prettier at any of these future milestones?

1. When we remove formatting rules in a future major version.
2. During a future rewrite of ESLint.
3. Never.

The main downside of adopting prettier is that it would pollute the git blame history for virtually every line in the codebase, although it's possible to workaround that by listing the prettier adoption commit to be ignored in a .git-blame-ignore-revs file alongside git blame --ignore-revs-file .git-blame-ignore-revs.

Participation

• [X] I am willing to submit a pull request for this change.

Additional comments

No response

[nzakas]

I'm not opposed to it, but as you mention, the disruption to the project won't be small. We'd need a solid plan to go forward, and ultimately, we may decide to only implement Prettier once we rewrite the core, where we can put it in from scratch.

How close to our current code formatting can we get Prettier?

[kecrily]

Our current code style seems to be very different from Prettier's, is this massive style change acceptable? It will destroy our existing git history

[bmish]

Doing some initial testing on this branch.

1. Add .prettierrc.js:

/** @type {import("prettier").Config} */
const config = {
    arrowParens: "avoid", // Current ESLint style.
    printWidth: 160, // ESLint sets max-len rule to 160. Setting printWidth to 160 resulted in a smaller diff compared to the default of 80. But note printWidth/max-len are not interchangeable: https://prettier.io/docs/en/options.html#print-width
    trailingComma: "none" // Current ESLint style.
};

module.exports = config;

2. Copy .eslintignore to .prettierignore.
3. Run via CLI (eslint-plugin-prettier doesn't support flat config yet):

npx prettier --write "**/*.js"

Output looks like: https://github.com/eslint/eslint/commit/8817ae6128613cb6726084513bf21b8cc55d24a2

The biggest change is that lines will get broken differently, some shorter and some longer.

[nzakas]

Hmm yeah, that's a lot. 🤔

I wonder if the best approach would be to just set up Prettier as a precommit hook so people are updating files as they make new PRs? That would hopefully minimize the turmoil?

And I definitely want to run Prettier on its own. It really shouldn't be run inside of ESLint.

[kecrily]

I'm concerned that when people make changes to existing files, the PRs submitted will have a lot of extraneous diffs that will affect the review.

[bmish]

I don't think a precommit hook would be desirable as it would risk polluting PR diffs with formatting changes and result in mixed/inconsistent formatting throughout the codebase. Plus, we can't really have ESLint's formatting rules (as today) and prettier enabled at the same time...

If we're going to do it, I'd highly recommend just doing a one-time cutover, which is how I've seen many codebases larger than ESLint successfully pull it off. The cutover could be timed for post-ESLint V9's release, after all those PRs have been merged and things have cooled down, to reduce merge conflicts and risk.

---

And I definitely want to run Prettier on its own. It really shouldn't be run inside of ESLint.

Sounds good as this is faster anyway.

---

Some stats about the prettier conversion commit:

• (ignoring whitespace) 683 changed files with 30,319 additions and 33,439 deletions.
• (including whitespace) 700 changed files with 53,320 additions and 56,440 deletions.

And codebase-wide line count stats:

❯ cloc . --exclude-dir "node_modules,tmp,coverage,.cache,build,docs,jsdoc,templates,fixtures" --include-ext=js  
     826 text files.
     815 unique files.                                          
      47 files ignored.

github.com/AlDanial/cloc v 1.98  T=0.78 s (1006.0 files/s, 520022.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JavaScript                     782          31622          50016         322588
-------------------------------------------------------------------------------
SUM:                           782          31622          50016         322588
-------------------------------------------------------------------------------

So I'm very roughly estimating that prettier affects ~10% of lines in the codebase.

And as I noted earlier, .git-blame-ignore-revs may help mitigate the git history churn.

[nzakas]

I'm unsure if this is actually worth the effort, but if other team members feel like this would be a good idea, happy to revisit after v9 is released.

[JoshuaKGoldberg]

FWIW I am strongly in support of this. As a relatively newer contributor it's been a pain point for me in ramping up to have to manually craft newlines/spacing/etc. Having a dedicated formatter such as Prettier would make contributing much more pleasant for me.

Separately, I can anecdotally say that I've never seen a diff change from introducing a formatter to be a noted pain in a project medium- or long-term as long as the .git-blame-ignore-revs file is added properly. Contributors can always manually apply the formatter to their PRs to avoid merge conflicts. This is especially true in projects such as ESLint that allow "unclean" PR histories rather than requiring strict rebasing.

Example reference: https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/65993. We've received zero (to my knowledge) user complaints about the introduction of a formatter. The only complaints I've seen have been from the side effect of publishing many packages - not a concern in ESLint.

[nzakas]

@JoshuaKGoldberg thanks for that context, very helpful.

For others, I found this reference: https://akrabat.com/ignoring-revisions-with-git-blame/

Given that capability, I'm on board with doing this. I'd say let's hold off and do it just before the final v9.0.0 is published.

[github-actions[bot]]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[nzakas]

It seems like there's general agreement to look into this, so let's plan on investigating after the final v9 is released.

Does anyone want to volunteer to drive this?

[JoshuaKGoldberg]

I'm happy to volunteer!

[nzakas]

We may do this as part of the work on https://github.com/eslint/eslint/pull/17876, so is probably worth holding off until we can finish up that PR.

[bradzacher]

Late last year I worked on migrating the codebase at work from dprint to prettier. The codebase has ~60k TS files and I touched every single one of them for a single commit that touched ~3.3 million lines.

Disruption to engineers was a key concern the company has a few thousand engineers and hundreds of active PRs open at any given time! The last thing I wanted was for everyone to be left in the lurch trying to figure out how to merge/rebase their changes across a formatting commit. People were excited for the reformat but that excitement would quickly turn to frustration/anger if they had to spend tens of minutes migrating each of their branches.

The solution we arrived at was to do the change across two commits instead of one - the first commit we landed all of the infrastructure, dependencies and configuration required to turn on prettier -- but we did not format the codebase. Put another way we turned it on without running it. The second commit landed immediately afterward and it formatted the codebase.

The reason we did this was because it gave engineers a clean commit to merge which would allow them to format their code without jumping through any hoops. This allowed us to provide a clear and simple set of instructions for engineers to update their PRs:

1) git checkout main && git pull 2) git checkout my-branch 3) git merge <CONFIG COMMIT> -q -m "merge prettier config in for reformatting" 4) run prettier on branch changes and commit format

• eg git diff --merge-base main | xargs yarn prettier --write 5) git merge main 6) git push

There was still, of course, some effort that engineers had to go through as git isn't great at dealing with conflicts - but the effort was much, much lower than if we'd just had one commit.

[bmish]

Limiting the automated formatting changes to a separate commit from everything else is a good idea. That makes it easier to ignore those changes for git blame and also easier to redo the formatting changes in the event of merge conflicts while waiting for the PR to be approved. But luckily, we likely won't have too many outstanding PRs to worry about once ESLint v9 ships...

[JoshuaKGoldberg]

Now that ESLint v9 is shipped and #17876 -> #18643 is merged, this is no longer blocked, right? Can I assign myself? 🙂

[nzakas]

@JoshuaKGoldberg go for it!