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
Change Request: Consider adopting prettier internally #17814
Comments
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? |
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 |
Doing some initial testing on this branch.
/** @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;
npx prettier --write "**/*.js" Output looks like: 8817ae6 The biggest change is that lines will get broken differently, some shorter and some longer. |
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. |
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. |
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.
Sounds good as this is faster anyway. Some stats about the prettier conversion commit:
And codebase-wide line count stats:
So I'm very roughly estimating that prettier affects ~10% of lines in the codebase. And as I noted earlier, |
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. |
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 Example reference: DefinitelyTyped/DefinitelyTyped#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. |
@JoshuaKGoldberg thanks for that context, very helpful. For others, I found this reference: 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. |
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. |
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? |
I'm happy to volunteer! |
We may do this as part of the work on #17876, so is probably worth holding off until we can finish up that PR. |
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:
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. |
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... |
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):
However, I'm raising this issue due to some recent developments:
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?
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 alongsidegit blame --ignore-revs-file .git-blame-ignore-revs
.Participation
Additional comments
No response
The text was updated successfully, but these errors were encountered: