Framework: Use Prettier?

[samouri]

Over the past few weeks, the prettier javascript auto-formatter has come up many times in calypso-framework chat. For anyone who wants a fun introduction to what prettier is, feel free to watch the creators lightning talk at ReactConf: https://www.youtube.com/watch?v=hkfBvpEfWdA

tldw:

• Prettier auto-formats your code so you don't need to (DevEx++). A linter like eslint cannot reliably fix style issues (and also doesn't ensure standard formatting around things like linebreaks).
• It reduces the learning curve for new OSS contributors
• It acts as a teaching tool by wrapping expressions according to javascript's order of operations
• React, Immutable, jest, babel, and more are all using it (prettier is well tested)
• It is not very configurable. We would need to give up our WP/PHP inspired spacing conventions and break with WP Core JS Guidelines. We would need to modify our eslint config to conform to prettier. e.g. eslint-plugin-prettier.
• The initial "run prettier" commit would mess with our git blame. GitHub makes it pretty easy to skip around blame history

I think we all know that we could debate styles until the end of time, so I figure lets just call for a vote and be done with it. Should we spin up a PR that formats all of calypso, modifies our eslint style rules, and updates our docs?

Thumbs up for yes, thumbs down for no.

[samouri]

ccing people that I've seen discuss it: @nb @gziolo @dmsnell @aduth @youknowriad @Copons @ockham

[gziolo]

There is also https://github.com/prettier/prettier-eslint which formats your code via prettier, and then passes the result of that to eslint --fix.

[gziolo]

It is also possible to execute prettier in a linter mode. It requires some code, but it shouldn't be a big deal. See this comment about Jest integration: facebook/react#9101 (comment).

[stephanethomas]

The initial "run prettier" commit would mess with our git blame.

I guess it depends on the scope of those changes but this looks like a non-negligible downside to me.

GitHub makes it pretty easy to skip around blame history

Could you point me to a good example of this? I always found that very difficult to do but maybe I'm missing something.

[dmsnell]

here's what I used for the notifications client:

prettier --print-width=100 --single-quote --tab-width=4 --trailing-comma=es5

in my IDE I have it auto-format on save which is wonderful because prettier doesn't just format code but it puts in things like parentheses and semicolons where needed. instantly I can see if I made a logical error due to something like precedence due to this auto-inserted syntax (removes code ambiguity).

on pre-commit it formats all of the changed files.

this has been an excellent way of removing a lot of cruft for me while trying to focus on more important matters. I think we could win in Calypso with it as well

there are things I don't like about it:

• some of the wrapping is ugly
• it has a few bugs (will work themselves out)
• it doesn't always do what I want, for example it won't allow one-item-per-line in small lists
• code diffs can unexpectedly grow because some added code leads to a formatting change (like single-line vs multi-line lists)

these annoyances have just been that: annoyances; I find the project as a whole is in a much better place because of it.

bias: I think JavaScript suffers by not having a standard format guide. I may not like the style but at least I don't have a choice… I use elm-format in the same way when I code in Elm and I like gofmt's stance and I use auto-format whenever possible

[dmsnell]

Could you point me to a good example of this? I always found that very difficult to do but maybe I'm missing something.

@stephanethomas ^^^

[stephanethomas]

Wow, I've never noticed that feature. Thanks @dmsnell!

[aduth]

A linter like eslint cannot reliably fix style issues

Why not? All Prettier does is parse the code into an AST and apply a pretty printer output. There is little difference between this and what ESLint does. While it may be true that ESLint's fixer doesn't currently support all rules, it's not fair to claim it can't.

It reduces the learning curve for new OSS contributors

Except if they're coming from a WordPress background.

It acts as a teaching tool by wrapping expressions according to javascript's order of operations

Sounds like a good candidate for an ESLint error rule.

React, Immutable, jest, babel, and more are all using it (prettier is well tested)

Is this more a point to them not having a consistent and documented style of their own previously?

[aduth]

Are we addressing a real pain with this proposal?

[samouri]

While it may be true that ESLint's fixer doesn't currently support all rules, it's not fair to claim it can't.

fair. ESLint may some day support fixing all rules

Is this more a point to them not having a consistent and documented style of their own previously?

The reason I brought this up wasn't about pointing out the merits or problems with their previous style. It was to highlight the level of maturity / expected continued development & support for prettier.

Are we addressing a real pain with this proposal?

The pain is totally subjective and will differ person to person. Its the reason I proposed a vote.

[dmsnell]

Are we addressing a real pain with this proposal?

for me we are. I would also argue that even if ESLint can "fix" these things it does a very poor job of it, which is one of the reasons given as the impetus for prettier. style isn't "fixed" or "broken" - it just is.

prettier makes it straightforward for us to completely remove that piece of coding from our minds when we're developing and for me that has been wonderful.

Except if they're coming from a WordPress background.

how is this? it seems like the jump from a "WordPress background" to Calypso is already huge, plus people have to learn our style guides. with an auto-formatter that last piece has been eliminated.

prettier itself just happens to be the first JavaScript auto formatter that I have seen that does an incredibly good job. it's based off of work that has been popular in functional languages for a long time. its does really well (despite the bugs because of its infancy) and seems like the final way to end the style questions

[aduth]

Except if they're coming from a WordPress background.

how is this?

Because despite some slight variation (that I'd like to champion upstream) and revisions specific to ES2015+, there's significant overlap between our style guide and the core JavaScript Coding Standards on which it was originally based.

[nb]

I love the developer experience of the AST approach brings to the table, though it should be able to match our current style and, to be honest, it would be great that some of the bugs are ironed out, because each change in the tool leads to formatting changes in commits. And we all love those :)

[dmsnell]

it should be able to match our current style

@nb I think we will be guaranteed that prettier won't ever match our current style. we have many deviations from it, many ambiguities, and a project goal is never having more than five configuration options.

[nb]

@dmsnell outside of the ambiguities, we should be able to get our spaces, at least.

[gziolo]

http://eslint.org/docs/rules/space-in-parens with --fix flag should help

[samouri]

@nb, you may be interested in: prettier/prettier#40.

In summary though: some vocal and influential parts of the js community think that it would be best to not support most configuration. The only one that will probably actually get developed at some point is tabs vs. spaces. Definitely not our liberal spacing around parenthesis.

[dmsnell]

see also https://github.com/prettier/prettier-atom and other tools

[ockham]

Seems like one key question in this discussion is: Do we want to, or do we not want to continue to follow WordPress core's JS Coding Standards? By using prettier (without any forks or tacked-on configuration), we would most certainly cease to do so.

[samouri]

Seems like one key question in this discussion is: Do we want to, or do we not want to continue to follow WordPress core's JS Coding Standards?

Agreed. A vote for 👍 is a vote for breaking with WP JS Coding Standards

[dmsnell]

Do we want to, or do we not want to continue to follow WordPress core's JS Coding Standards?

I still think the link and the value of the link is dubious. Our coding style is considerably different than what one will find in WordPress and that's happening at a much higher level than spaces and line-lengths.

A vote for this is also a vote to remove the obstacle of needing to learn any coding style at all, to eliminate all discussion about style, to handle styling deterministically, and to enjoy the benefits thereof 😄

[samouri]

I've created a PR to figure out the finer details in case we decide to Use Prettier #12479

[oandregal]

I came here to ask for a test case, as I'm still educating myself on the impact/caveats/pros-cons this would have, so ❤️ for #12479 @samouri

[gziolo]

If we introduce prettier we should also mitigate issues with formatting when using codemods. See related discussion: #12521 (review).

[johnHackworth]

To be honest, I don't think we should vote to decide if we deviate from WP-core standards ... I think this is the kind of decision it should come "from above", or, in any case, decided between the whole company (if we change our standards, we should do it in every automattic project, not just in calypso). A one-sided calypso change would be pretty terrible for anyone who usually works on more than one of our projects at the same time

[samouri]

decided between the whole company (if we change our standards, we should do it in every automattic project, not just in calypso). A one-sided calypso change would be pretty terrible for anyone who usually works on more than one of our projects at the same time

I disagree. As @dmsnell explained higher up, using prettier actually doesn't require you learn much new. Its a one time setup cost of integrating a new plugin to your editor/workflow. This shouldn't make working on multiple projects any harder.

[blowery]

https://github.com/prettier/prettier-eslint seems like it'll get us where we want to be format wise.

[gziolo]

👍 to what @blowery said. Prettier + eslint --fix will do the trick.
I expect it won't work perfectly with max line width set by prettier, but it shouldn't be an issue for us. It's an edge case. We can add an integration with IDEs first to allow people to play with this tool. If it turns out to be useful we can add pre-commit hook and CircleCi verification.

[blowery]

related prettier/prettier-atom#133

been trying that out with our !.eslintrc.js line removed from .eslintignore. Seems to work fairly well. It doesn't currently fix {foo} to { foo }, though eslint seems to support autofixing that one. Also doesn't trigger eslint to error...

[samouri]

Because I've been wanting to use it locally for development, I created a fork (wp-prettier) that keeps our spaces and uses tabs. In order to setup in vscode I did this (and installed the prettier plugin):

npm i wp-prettier
npm i --save-dev prettier 
cp node_modules/wp-prettier/src/* node_modules/prettier/src

[dmsnell]

I created a fork (wp-prettier)

hrm… not sure how I feel about that but good work!

[gziolo]

As mentioned here #13759 (comment). If we want to effectively use codemods it would be very convenient to have prettier in our workflow. Otherwise we will have to fix issues manually from time to time, because I don't believe eslint --fix is smart enough to resolve all styling issues.

[dmsnell]

for the sake of discussion, I would like to propose that we don't mix eslint formatting and prettier. if we introduce prettier then we should stop using eslint altogether for any of its formatting work. leave it all to prettier and leave eslint exclusively for code linting.

why? prettier is designed to be a deterministic formatter and if we start mixing it we're not only defeating that purpose but also introducing potential races and cycles in the formatting between them.

[samouri]

I've opened a PR to incorporate a fork of prettier into calypso as a devDependency. @blowery, @bluefuton, and I have already started using it within the Reader codebase. It was especially helpful with removing all of our usages of React.createClass here.

cc @gziolo

Edit: forgot to link to the pr #14010

[gziolo]

There is ongoing discussion about using prettier in WordPress core. There is similar set of issues that need to be resolved. @ntwb is planning to explore this further.

[gziolo]

I found also this closed issue in Prettier's repository: prettier/prettier#1303. It seems like they aren't totally against this rule. If there would be a bigger group requesting this feature they would at least consider it.

[jsnajdr]

Over the last few weeks, I spent some time playing with our prettier fork (originally created by @samouri) and I did some work that might be interesting for Calypso and WordPress developers:

I turned the changes into a proper option (--paren-spacing) that can be enabled or disabled. This makes the patch at least theoretically mergeable into the official version. If the maintainers ever decide to include the option, we have a PR ready.

I rebased the patch two times already against official releases -- first time from 1.1 to 1.2, second time 1.2 to 1.5. It's not a very big deal and I can commit to doing the upgrade regularly as needed and to fix any related bugs.

There is a calypso-1.5 branch in the fork repo that contains a sequence of independent and easy-to-maintain commits. One adds the --paren-spacing option, another sets the Calypso preferences as defaults and ignores any configuration passed from outside, another updates the README.md.

If we decide to use the fork, I think it's now in a very good shape and easy to maintain.

I don't know the status of the WordPress core discussion mentioned by @gziolo and the explorations done by @ntwb. I'd love to learn more about that.

[dmsnell]

Thanks for maintaining that @jsnajdr - it's still a big patch and I'm not sure how easy it would be to maintain if someone else had to jump into it, especially if the official repo diverges much.

I'm more in favor every day of just accepting all the default options and letting it be. We even recently made the jump to spaces over tabs in the notifications client (and I prefer tabs!) and so far it hasn't caused one little headache at all. The benefits of not having to think about styling are so huge and the auto-format while I code makes me that much quicker.

[jsnajdr]

I'm more in favor every day of just accepting all the default options and letting it be.

If that ever happens and the style guide for Calypso (or WordPress in general) changes, that's going to be great, of course. Meanwhile, if someone wants to just press ⌃⌥F in their editor today and get the code formatted, the fork seems like the best option for that. And that option just got a little bit better.

[blowery]

@jsnajdr is there a PR out there to update calypso to use the newest fork?

[jsnajdr]

is there a PR out there to update calypso to use the newest fork?

Just created one: #15942

[samouri]

lets use calypso-prettier

With the release of React 16, and the important role that codemods will play in getting us to a state where we can upgrade, I think its important to revisit the question of whether or not we want to use a code auto-formatter for the entire repo. Besides for all of the points mentioned in this thread so far, the one I'm focusing on right now is that running calypso-prettier on the entire wp-calypso codebase will make running codemods significantly easier.

Here are some updates on calypso-prettier and its usage within Calypso.

1. We've updated to 1.6. prettier seems to have a monthly release cycle so far. We now have a very smooth process of upgrading. After merging in a prettier upgrade, we simply run npm run reformat-files.
2. It now supports prepending a @format docblock to any modified file. This allows us to know whether or not a file's formatting is being controlled by prettier. This functionality is also in the process of being merged back upstream to prettier proper.
3. We have a pre-commit hook that auto-formats any file containing the @format docblock to ensure that an automatically formatted file stays automatically formatted
4. We've modified eslines to ignore formatting rules for any file containing the format flag
5. Roughly 10% of client is currently being managed by calypso-prettier.

Thanks to all the people that have been making calypso-prettier possible: @jsnajdr, @sirreal, @nosolosw, @dmsnell, @jsnmoon, and @blowery (I might have missed someone).

Note: @dmsnell attempted this months back: #14801

Related: p4TIVU-89g-p2

[frontdevde]

Speaking from my recent trial perspective, I have to say that calypso-prettier was immensely helpful in getting up to speed with the formatting style. It meant I could focus on producing output right away rather than worrying about formatting details (while learning them along the way by looking at the changes made by calypso-prettier).

Two key advantages here: It makes the onboarding experience for new OSS contributors considerably smoother. It cuts down the time spent on PR reviews considerably, no more loops just to correct formatting means time saved on both ends.

So from a trial / new hire perspective definitely a 👍 for calypso-prettier.

[sirreal]

My evolution with prettier:

1. Prettier seems cool, never used it.
2. Oh, we can use prettier in Calypso? Let's try it.
3. Wow prettier is handy, I just write code and indentation, spacing, etc. don't break my flow. That feels good!
4. I really with this file I'm working on for this PR was using prettier. I'll use it and just stage the lines I'm changing in 😎
5. I can't live without prettier! Every PR is now 2:
   1. prettier X
   2. Make actual changes to X

My vote is yes!

[dmsnell]

My evolution with prettier:

1. gofmt was the greatest decision made in that language
2. elmformat is so lovely
3. Why is eslint so bad with formatting?
4. Wadler? jlongster? prettier? Community-wide JS code standards? AWESOME!
5. IDE -> Run on Save -> prettier
6. Please please please can we adopt this yesterday?

My vote has been openly yes!

[sirreal]

Shall we call this one closed by #18453 #18282?

[gziolo]

It finally happened! 🎉