Use prettier for Kibana

[epixa]

We discuss/argue/change our coding style a lot, and we're always finding new style things to discuss/argue/change. Consistency is important, but exact styles we settle on aren't necessarily very important.

I propose that we start using prettier to manage code formatting entirely. Prettier automatically formats code for you, so you don't need to worry about exact format requirements or anything like that. One less thing we need to bikeshed about every 3-6 months. Other languages like go have utilities built in for this, but we're stuck with userland stuff in JavaScript. In any case, prettier certainly has substantial appeal as it is used by a ton of projects out there like react, babel, yarn, and webpack.

This wouldn't remove the need for eslint entirely because prettier only deals with code formatting. It doesn't do things like verifying import locations, but it should make discussions about indentation and whatnot a thing of the past.

[epixa]

@kjbekkelund You've been using prettier for a few months now in the new platform, what are your opinions about it?

[kimjoar]

So incredibly many +1s. It's amazing to not have to think about those style issues. It's not always perfect, but it gives great results for almost everything.

The only pain is of course that it will be a freakin' huge commit, so we need a great explanation on how to handle it in PRs etc. I've been through some large style changes like this before, and it takes a bit of time and pain to get it in.

[epixa]

If people run prettier on their PRs and squash their commits to one on top of the latest master, won't that avoid most conflicts? We could test this out to verify, but it should. The commit will be huge, but it should probably be much less impactful on open PRs than other style changes we've done.

[kimjoar]

I expect there will still be quite a few conflicts, but hopefully not too bad. We just need to do some testing and write up an explanation for people ("do this and it won't be too bad").

[epixa]

I experimented a bit with a refactor-related PR (https://github.com/elastic/kibana/pull/14165), and I can confirm that a prettier change causes a lot of conflicts.

These are the steps I followed:

1. Make sure feature branch is based on the latest non-prettier'd commit of master
2. Run prettier on all files in master
3. Run prettier on only the files modified in the feature branch
4. Commit this new prettier change to feature branch and then squash all commits in feature branch into one commit
5. Rebase on prettier commit in master

I figured this would basically make the diff only deal with post-prettier results and thus wouldn't trigger too many random conflicts. I was wrong.

So we probably want to carefully time and coordinate the change to minimize the pain, but I still think this is a good option. The pains we're talking about are short term, and the benefit is long term consistency in formatting without squabbling over the details.

[epixa]

It is worth noting that the out-of-the-box formatting rule for prettier uses double quotes, and while that can be changed to single quotes, I intentionally didn't change it in my experiments because I wanted to see some worst-case level conflicts. It's possible we could opt into single quotes and such for the sake of making the move easier for open PRs.

Personally, I'm keen on not changing defaults wherever possible just so we don't get into a mode of arguing about prettier tweaks, but if there is a a technical or logistical justification for something up front, then so be it.

[kimjoar]

@epixa Haha, yeah, that's the same experience I've had before re conflicts. I think going single quotes will definitely make it easier to get in. I'd prefer using defaults, but if it simplifies conflicts I think that one option can be worth it.

[epixa]

In terms of a roll out plan, we could do this on a plugin by plugin basis based on the current open pull requests and whatnot. I haven't attempted to evaluate the open PRs like that, but it is an option if necessary.

[kobelb]

@epixa do you have a plan for how we'll deal with backport branches? Would we run prettier on all actively "supported" branches?

[tsullivan]

Is there any chance that the code that prettier generates would break one of our many ESLint rules that we have around whitespace?

[epixa]

@kobelb Yes, assuming this happens post 6.0 GA, I think we'd have to backport this to 6.x/6.0/5.6.

[epixa]

@tsullivan Yes, definitely, but you can have prettier run automatically via your editor and such, so I figured we'd just remove essentially all of the linting rules around code formatting.

[tsullivan]

I did some playing around and it looks like we can set options to Prettier that will make its code conform to our ESLint rules very easily.

[sorenlouv]

Just want to add a +1 from here. APM has been using prettier since we started, and I rely so heavily on it now, that it's the first thing I add when starting a new project.

Is there any chance that the code that prettier generates would break one of our many ESLint rules that we have around whitespace?

We've added our own .eslintrc.json, which enforces prettier styles, and overrides conflicting rules.

{
  "extends": ["prettier", "prettier/react"],
  "plugins": ["prettier"],
  "rules": {
    "prettier/prettier": [
      "error",
      {
        "singleQuote": true,
        "semi": true
      }
    ]
  }
}

Regarding roll-outs, I'd suggest asking each team to add a locale ESLint configs in their plugin folder and do the migration themselves. Then in x weeks, we can remove the locale configs, and run prettier on the entire project (in case there is code without ownership, and to ensure consistency). This way we don't need a big bang roll out plan that might interfere with people's roadmaps and long-running branches.

[kimjoar]

Btw, Prettier supports Markdown and CSS too. So we should be able to run it on most things in the code base.

[cjcenizal]

we'd just remove essentially all of the linting rules around code formatting

Based on the 15 minutes I've spent learning about Prettier, it seems like the max line length is what drives the formatting, right? I ❤️ this but I also love consistent formatting (sometimes in spite of line length) for making code readable and reducing noise in diffs. For example...

const point = { x: 10, y: 20 };

So far, so good but as we add additional information to the point, it will read the max line length and wrap.

const point = {
  x: 10,
  y: 20,
  z: 1247,
  speedX: 0.34,
  speedY: 97.57,
  speedZ: 6349.09,
  mass: 35,
  width: 30,
  height: 349,
  depth: 689
};

This still looks good, but if we have multiple points now the formatting changes depending on each object.

const pointA = {
  x: 10,
  y: 20,
  z: 1247,
  speedX: 0.34,
  speedY: 97.57,
  speedZ: 6349.09,
  mass: 35,
  width: 30,
  height: 349,
  depth: 689
};

const pointB = { x: 3, y: 6, z: 3002 };

Personally I find it easier to scan similar objects if they're formatted similarly.

Also, if we edit PointA to have fewer properties, then Prettier will reformat it to fit on one line, creating a diff which is a bit more difficult to parse.

I don't have any solutions for these issues but I thought I'd raise them to see if anyone else feels the same way. Maybe there are some ESLint rules we can retain that could help us here, per @tsullivan's point?

[sorenlouv]

@cjcenizal In general you lose a lot of freedom in how you want things formatted. It's a little annoying at first but just give in - after a while you won't miss your freedom ;)

Object literals are an exception and you do have a bit of wiggle room. The above can still be formatted like this if you want:

const pointA = {
  x: 10,
  y: 20,
  z: 1247,
  speedX: 0.34,
  speedY: 97.57,
  speedZ: 6349.09,
  mass: 35,
  width: 30,
  height: 349,
  depth: 689
};

const pointB = {
  x: 3,
  y: 6,
  z: 3002
};

You can play around with it here: https://prettier.io/playground/#N4Igxg9gdgLgprEAuc0DOMAEAHCBLWAQUwF5NgAdKTTADyUwEYAGAGipoE8GAmNjzAC8GjHgBYA7O2qY02OHAAmADQbMAdAGYx0mnIWKAmgwCcE9QFYpA-UoBaDAGzaT65id2YAtgEM0aBk0LTwB3PEUYAAtA-hlIuDwAc0iYQLEPAUU4bCinAA4TKgBfAG4qKkgoDBx8WAAhUnJMAXpMTVZMbkxHDuE25mYeTFKQVhAIHLx0ZFA-GGQAMx8AGzQ4MYAjACcfMABrOBgAZWxdgkTkGC2AV3WQRQgwRZW1sYArNFo6nf3Do58vHAADIEODPVZ3CDXGDYaE8cGvECnLZrLbIEAbHwbTjLaCjJFbAgwADq4SiyDybBAay8eEuNzuaHOyzgAEVrhB4Ai7jAsaSIpFkDwxlcfHhlucAMIQLy+dFQaBgsbXNYAFSxaG5RSKQA

[cjcenizal]

@sqren Oh nice! Thanks for the link. I do like that Prettier takes formatting out of our hands which is the core value prop. As long as we can define some rules to retain code readability, I'm happy. 😄

[rhoboat]

How do folks feel about configuring prettier to have trailing-comma: all ? For reference: https://prettier.io/docs/en/options.html#trailing-commas

We can then close https://github.com/elastic/kibana/pull/14631

[tsullivan]

How do folks feel about configuring prettier to have trailing-comma: all ? For reference: https://prettier.io/docs/en/options.html#trailing-commas

I'm +1 for it.

[sorenlouv]

@archanid

How do folks feel about configuring prettier to have trailing-comma: all ?

I'm okay with this change - but I'd rather stick to the defaults, if the alternative is to bikeshed all the options :) 🚲

[epixa]

I'd also prefer to stick with the defaults. To me, the most valuable part of this change is removing the periodic formatting discussions that crop up that result in us tweaking our linting rules. This is only the case if we don't just trade eslint formatting discussions for prettier flag formatting discussions.

If we are to accept some non-default options for our prettier usage, then let's make sure to ground them in concrete rationalizations so we don't get bogged down in subject crap like "is more readable". And ideally these rationalizations should be applicable beyond any one specific circumstance.

[epixa]

For a rollout plan, what do folks think about @sqren's suggestion here? https://github.com/elastic/kibana/issues/14810#issuecomment-342643061

If teams make sure to consider open PRs for their plugins/code in kibana and x-pack, then this sounds like a good approach to me.

[kimjoar]

++ I like it.

We can also add a Prettier task at the root of the project (npm run prettier), so you don't need to remember where to run Prettier before pushing. To begin with it can .prettierignore all the things, then gradually as we move stuff over we remove ignores from that file.

[mattapperson]

I am all for this plan, esp with @kjbekkelund's suggestion!

[mattapperson]

OK, so I whipped up a quick PR to do this (plus ensure prettier opted-in sections stay formatted correctly). If it is not accepted that is cool, just wanted to get the ball rolling :)

[kimjoar]

This is implemented in https://github.com/elastic/kibana/pull/16514.

Want to add Prettier to your part of the code base? Check out https://github.com/elastic/kibana/blob/e58b43f744b1325619d6d4de7b33892bb575e6b5/.eslintrc.js#L19-L22 to see how it’s done (and sometimes you might need to add a line to the .eslintignore in the same folder too). Then run node scripts/eslint --fix and you should be good to go!