Framework: Use Prettier?

samouri commented 7 years ago

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 commented 7 years ago

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

gziolo commented 7 years ago

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 commented 7 years ago

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: https://github.com/facebook/react/pull/9101#issuecomment-286610833.

stephanethomas commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

navigatingblames

@stephanethomas ^^^

stephanethomas commented 7 years ago

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

aduth commented 7 years ago

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 commented 7 years ago

Are we addressing a real pain with this proposal?

samouri commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

gziolo commented 7 years ago

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

samouri commented 7 years ago

@nb, you may be interested in: https://github.com/prettier/prettier/issues/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 commented 7 years ago

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

ockham commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 :smile:

samouri commented 7 years ago

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

oandregal commented 7 years ago

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 :heart: for #12479 @samouri

gziolo commented 7 years ago

If we introduce prettier we should also mitigate issues with formatting when using codemods. See related discussion: https://github.com/Automattic/wp-calypso/pull/12521#pullrequestreview-29108476.

johnHackworth commented 7 years ago

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 commented 7 years ago

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.

mattwiebe commented 7 years ago

The WP JS standards were adopted from the jQuery project with a slight eye towards being not too strange to WP PHP devs during WordCamp Boston in 2013. Most code in core was jQuery spaghetti, with only the Customizer existing, and the Backbone-based media library being in active development.

That was a JS epoch (or two) ago, and I would feel bad if they held us back from doing what makes the most sense for Calypso.

gziolo commented 7 years ago

They released stable version of prettier: http://jlongster.com/prettier-1.0. In fact it's already v1.1.0. It contains a new option tabs vs spaces (https://github.com/prettier/prettier/pull/1026), which was missing before.

johnHackworth commented 7 years ago

I disagree. As @dmsnell explained higher up, using prettier actually doesn't require you learn much new.

It's not just about writing code in the way you like... it's about the overhead of reading different codebases with different styles daily, and mainly, it's about education and getting used to the style automattic uses everywhere.

Don't get me wrong, I'm not the biggest fan of our style guide. I joined Automattic as a javascript wrangler because I've worked mainly as a javascript developer for years... and all the style guides I've been familiar with were totally different to WordPress one. It's a rough ride at first ( my first calypso PR had more than 100 comments... most of them about style guide violations :scream: ), but you learn it and you get used to it ( to the point to start using spaces inside parens even when you are writing prose ;) ) . So I'm not especially fond of our style guide, no. But does it mind? No, not at all... I think that pretty much any style guide is as valid as any other, as long as you are consistent and make sure people follows it.

And what it will happen if we make calypso SG to depart from WP standards? We would be breaking this consistency. We would have different javascript projects using different styles. And what is worst, we will lose our cross-language consistency.

It's not perfect, of course, but right now you can read the PHP WordPress.com API code and then jump into calypso code and the style is pretty similar. The PHP style is easy to learn for newbies who come from calypso because a big chunk of it is exactly the same they already used for javascript. And this is quite critical: Most of the people who start developing in calypso are javascript developers who are not very familiar with WordPress PHP standards (that was my case, and I'm pretty sure it's the case for the majority of us, javascript wranglers). And by learning calypso standards, you get used to the set of rules you will need to use when you do the jump and start working also on the API side. If we move calypso standards away from WP ones, we will be throwing every new person to the wild wild west world of the API (no linters, much looser code reviews, etc) without even being familiar with the code style rules.

All this could be ok if it was the counterpart of something big. But what do we win with moving to prettier / changing the style guide? Our automattic linter is already very good (major kudos to @aduth and everyone who worked on it), and even if Prettier could be better in some fringe cases, we are talking about getting to a 97% good platform to a 100% one, not about fixing some process that is currently broken. The only other major point that could be made is that moving to a more js-scene friendly set of rules could open the road for new OSS contributors ... and that's a good point and it would be amazing. But being honest, we don't have any evidence that it would happen, and it's a pretty major step to take just to test if it works.

samouri commented 7 years ago

Don't get me wrong, I'm not the biggest fan of our style guide.

This whole paragraph seemed like an argument for prettier ;)

it's about the overhead of reading different codebases with different styles daily

Personally, I find that in the JS world I do that every day anyway. Calypso has many external OSS dependencies (redux, react, etc) and each of those follow a radically different style than our own.

It's not perfect, of course, but right now you can read the PHP WordPress.com API code and then jump into calypso code and the style is pretty similar.

You bring up a point I hadn't really considered before. I personally haven't don't much wp-api work yet so don't feel comfortable weighing in here.

But what do we win with moving to prettier / changing the style guide?

This is essentially the same argument @aduth already brought up with: "Are we addressing a real pain" and @dmsnell answered that here: https://github.com/Automattic/wp-calypso/issues/12260#issuecomment-287445965.

blowery commented 7 years ago

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

gziolo commented 7 years ago

:+1: 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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

I created a fork (wp-prettier)

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

gziolo commented 7 years ago

As mentioned here https://github.com/Automattic/wp-calypso/pull/13759#issuecomment-300390056. 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 commented 7 years ago

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 commented 7 years ago

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 https://github.com/Automattic/wp-calypso/pull/14010

gziolo commented 7 years ago

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 commented 7 years ago

I found also this closed issue in Prettier's repository: https://github.com/prettier/prettier/issues/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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

jsnajdr commented 7 years ago

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

Just created one: https://github.com/Automattic/wp-calypso/pull/15942

samouri commented 7 years ago

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.

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.
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.
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
We've modified eslines to ignore formatting rules for any file containing the format flag
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: https://github.com/Automattic/wp-calypso/pull/14801

Related: p4TIVU-89g-p2

frontdevde commented 7 years ago

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 :+1: for calypso-prettier.

sirreal commented 7 years ago

My evolution with prettier:

Prettier seems cool, never used it.
Oh, we can use prettier in Calypso? Let's try it.
Wow prettier is handy, I just write code and indentation, spacing, etc. don't break my flow. That feels good!
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 😎
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 commented 7 years ago

My evolution with prettier:

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

My vote has been openly yes!

sirreal commented 7 years ago

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

Automattic / wp-calypso

Framework: Use Prettier? #12260

lets use calypso-prettier