💅 Add Prettier 💇

[rwjblue]

rendered

[kpfefferle]

The current package.json blueprint includes a generic lint script command which uses npm-run-all to run all lint:* script commands.

How would this work with this RFC's proposed lint:fix script command, which similarly uses npm-run-all to run all lint:*:fix script commands? Would running the existing lint run both lint:fix and each individual lint:*:fix script, or does something prevent each lint:*:fix command from running more than once?

I would assume we'd want to avoid running duplicate lint tasks, but I do like that the wildcards in these script commands conveniently pick up on similarly-named script commands that application authors may add (such as lint:ts).

[lifeart]

Should we add prettier for handlebars templates too?

[rwjblue]

How would this work with this RFC's proposed lint:fix script command, which similarly uses npm-run-all to run all lint::fix script commands? Would running the existing lint run both lint:fix and each individual lint::fix script, or does something prevent each lint:*:fix command from running more than once?

Great question @kpfefferle! I pushed an update in a32ad2d to clarify and show how to fix (tldr; you change from lint:* to lint:!(fix)).

[rwjblue]

@lifeart:

Should we add prettier for handlebars templates too?

I don't think we should do that at this time, but as the formatting behaviors continue to improve in .hbs files we should reconsider (follow along with that progress here). IMHO, doing this should follow essentially the same pattern that this RFC currently lays out for ESLint (adding a ember-template-lint-plugin-prettier which will implement a --fix).

[Alonski]

@lifeart Prettier for HBS files isn't completely ready for the limelight yet in my opinion. There are still a few kinks to remove before we can do this. :)

[lifeart]

Prettier quest: prettier/prettier#4908

[Gaurav0]

The RFC adds lint:hbs:fix. This probably should be added regardless of whether prettier configuration is added.

In my experience with prettier, it generates lots of stylistic errors that often obscure "real" problems as well as issues I might temporarily not want autofixed (like debugger statements and console.logs). Is there a way to temporarily disable prettier errors on the command line? If so, can we add that to the config?

Is there some way we can add a git precommit hook to the boilerplate as well to automatically run lint:js:fix? I often break prettier linting with only minor touchups and forget to rerun it.

[rwjblue]

@Gaurav0

The RFC adds lint:hbs:fix. This probably should be added regardless of whether prettier configuration is added.

Sure, but we have to add it somewhere. I guess I could remove lint:hbs:fix from this RFC because it is not prettier related, but that doesn't really seem like a great option to me.

In my experience with prettier, it generates lots of stylistic errors that often obscure "real" problems

Perhaps, but FWIW, that is also the part that is super freeing for developers. As I mentioned in the RFC, you just author however you want to and let Prettier take care of the final formatting. Editor settings such as format on save or suggested code actions make it very trivial to automatically apply, and if you don't want to fiddle with your editor settings the yarn lint:fix is only a short terminal hop away.

as well as issues I might temporarily not want autofixed (like debugger statements and console.logs).

Prettier will not remove debugger statements (and neither will ESLint as of eslint/eslint#10509). The experience you had was almost certainly the issue reported in eslint/eslint#10242.

Is there some way we can add a git precommit hook to the boilerplate as well to automatically run lint:js:fix? I often break prettier linting with only minor touchups and forget to rerun it.

I personally dislike precommit hooks as a rule, and violently dislike those that rewrite file contents. In theory we could add one that validates only (see this example), but I'd prefer not to.

[jherdman]

This was/is a holy war for my team. Including Prettier was massively disrupting at first, though we have experimented with it on greenfield projects. That said, why Prettier? Why not Standard? Etc.

[rwjblue]

@jherdman

Including Prettier was massively disrupting at first

Ya, I totally agree that initial adoption is definitely the most difficult part of the process. One thing to keep in mind is that this RFC is specifically talking about adding this setup to new apps/addons (e.g. greenfield projects). While it is true that ember-cli-update would pick these changes up and attempt to apply them to existing apps/addons, it is easy enough for the person doing the upgrade to clear those specific changes and hold off on them (once you do that for the first version that includes the changes, ember-cli-update is smart enough to avoid suggesting them again).

Why Prettier? Why not Standard? Etc.

First up, I'll be perfectly frank: I chose Prettier for this RFC because I prefer it and have the most experience with it. Aside from my personal preferences Prettier is much more popular with 9,249,847 weekly downloads vs 265,658 weekly downloads (we all know these download numbers don't mean a ton, but the order of magnitude can tell us something), and (aside from the defaulting of singleQuotes) is much more aligned with the Ember ecosystems inherent preferences.

Another data point here is that a very large number of the Ember ecosystem projects are already using Prettier internally. These include ember-source, ember-cli, ember-data, @ember/test-helpers, eslint-plugin-ember, the various packages composing the rendering engine, @glimmer/component, ember-template-lint, and many more. Additionally a large number of community maintained addons are also using it already (here is a listing using EmberObservers awesome code search feature).

[ballPointPenguin]

FWIW I have established a pattern in my apps for using eslint-config-prettier. It's nice when prettier rules and linting (and fixing) are managed by eslint, in my experience, so that they are never in conflict and existing tooling around eslint (e.g. Text Editor plugins and pre-commit hooks) just work.

for reference
https://github.com/ballPointPenguin/impulse-sieve/blob/master/package.json
https://github.com/ballPointPenguin/impulse-sieve/blob/master/.eslintrc.js

I also include standard config, but that would be more disruptive to hoist onto the general public (no semicolons!)

[rwjblue]

@ballPointPenguin

FWIW I have established a pattern in my apps for using eslint-config-prettier. It's nice when prettier rules and linting (and fixing) are managed by eslint, in my experience, so that they are never in conflict and existing tooling around eslint (e.g. Text Editor plugins and pre-commit hooks) just work.

Yep, totally agree! Thats exactly the setup proposed here. 😃

[rwjblue]

We discussed this in todays meeting, and are moving into final comment period.

[ming-codes]

Doesn’t this overlaps with the current editorconfig?

[pichfl]

Big fan. Prettier caused and removed quite a bit of discussion in many of my projects but it was worth it for the less noisy PRs alone.

1. Why package.json over .prettierrc.js? We have .eslintrc.js and so on as well, I think it's a bit odd that this config is moved into package.json:

module.exports = {
  singleQuote: true,
};

2. While it is still experimental, Prettier does support hbs to some extend. Would it be good to show an example config?

module.exports = {
  singleQuote: true,
  overrides: [
    {
      files: '**/*.hbs',
      options: {
        singleQuote: false,
      },
    },
  ],
}

3. Using single quotes in yaml files will lead to issues in project which use ember-intl and other plugins that rely on yaml files with ICU translation syntax, which uses single quotes for escaping (no clue why).

module.exports = {
  singleQuote: true,
  overrides: [
    {
      files: '**/*{yml,yaml}',
      options: {
        singleQuote: false,
      },
    },
  ],
}

All three blocks merged make up the Prettier config that served me well in all recent ember projects.

[Gaurav0]

This was introduced 4 days ago and has had considerable feedback. Why is it being rushed into FCP?

[rwjblue]

@ming-codes

Doesn’t this overlaps with the current editorconfig?

@ro0gr asked a similar question inline, but the thread is resolved so it doesn't display. Here was my reply:

Are we going to respect editorconfig settings with prettier?

Yep, Prettier already parses and uses the .editorconfig values for some of its defaults. It is a bit buried, but see these docs:

If options.editorconfig is true and an .editorconfig file is in your project, Prettier will parse it and convert its properties to the corresponding prettier configuration. This configuration will be overridden by .prettierrc, etc. Currently, the following EditorConfig properties are supported:

• end_of_line
• indent_style
• indent_size/tab_width
• max_line_length

[rwjblue]

Thanks for chiming in here @kellyselden!

I don’t find that Prettier makes using git blame much harder other than the initial migration to Prettier. I do a ton of git archeology both in editor and in GitHub (knowing the “why” for a given change / logical condition / etc is super important before you change it). In both locations, having to traverse to history prior to the most recent change is extremely common (unrelated to Prettier) and IMHO very often fundamental to understanding the code properly before modifying it. Both GitHub’s UI and in editor tools (Magit for Emacs, fugitive for vim, GitLens for VS Code are ones I’ve used extensively) make it pretty easy to navigate the git history. In the end, it is very important for this kind of deep dive / investigation to not be surface deep (e.g. only looking at the most recent commit/blame entry).

That initial migration absolutely takes over the git blame output changing many lines of code that may not have previously been touched, but part of the goal of this RFC is to ensure that initial migration never has to happen (because we add it for you to begin with). After that initial migration is competed Prettier rarely modifies lines of code on its own that were not otherwise being modified by a human (though some early Prettier versions flip flopped a bit on syntax, so upgrades had a bit more noise while they figured out their process / policies).

[rwjblue]

@kellyselden - Finally back to my computer, and wanted to reply to a few more points you made:

I find it interesting there was no reference of the previous discussion around adding and reverting this ember-cli/ember-cli#7458, unless I'm missing something.

FWIW, initial drafts had a bit of a "history lesson" vibe, but it just wasn't very interesting or useful to the overall proposal. Your point in that PR was 100% correct: a change of this magnitude and impact to the community deserves an RFC. That is what this RFC is. 😸

There was absolutely no malicious intent or burying of facts in leaving it out.

I end up having to think about formatting and how prettier is going to uglify the code while I'm writing to try and get it to format it in a certain way, which breaks my concentration and flow.

I think this is likely the main point of disagreement here. The entire point of this proposal (and AFAICT Prettier itself) is exactly the opposite: folks should not think about formatting at all when authoring or reviewing code. Your brain power is much better used on solving actual problems in your domain, the computer can do the rote formatting for you.

In my experience, having bespoke formatting works when only one person is responsible to maintain it, but once you have a team of folks all with their own "style" you have:

• A large hodgepodge of styles in the same repo, making it quite difficult to know for some new addition what its style should be
• Pull requests / merge requests get stuck in a quagmire of argument about formatting, when they should be focusing on the code / feature / etc.
• Lots of wasted cycles discussing, changing, removing, custom bespoke styles

tldr; I believe that in the end, even if you disagree with Prettier's formatting of specific constructs it still lets you build higher, faster, with less team confusion and conflict. THAT is why it is especially important for the Ember ecosystem to adopt.

[rwjblue]

I’ll work on an update to move from package.json to stand alone files (likely will be a day or so to get that update pushed up, doing a bit of construction over the long weekend).

FYI - I've done that in 1276199.

[kellyselden]

Prettier rarely modifies lines of code on its own that were not otherwise being modified by a human

That's exactly the problem I'm refering to. Prettier often, in my experience, takes a simple change and makes it hard to trace in history. Given the example:

const aListOfStuff = [
  'item1',
  'item2',
  'item3',
  'item4',
  'item5',
  'item6',
];

Now removing item6 gives you:

const aListOfStuff = ['item1', 'item2', 'item3', 'item4', 'item5'];

Now trying to figure out when item2, for example, change takes two blame lookups. This is a simple example, you can imagine it gets pretty hair in real-world. This is what I meant when I said prettier makes me think of formatting while I'm coding, and the historical context change if I make certain changes to code.

All your responses to me seem like you think I'm arguing against the value of linting and autofixing. That's not true. I'm informing people that if you adopt prettier, you're probably going to lose reliable git blames, which was the case for me. (You also will get some pretty awkward one-liners, think array lists, destructuring, etc)

[NullVoxPopuli]

The array example, and maybe small objects, are the only ones I know of where I personally prefer the non-prettier version.

Though, I still prefer prettier overall.

If prettier could instead have some way of determining when multi-line arrays can be formatted to look nicer... Then I'd have 0 problems with prettier.

But for my case, I had an array with a grid of 5x5 entries in the array, and prettier made it 25 lines.

:Shrug:

Idk. There seems to be no way to programmatically figure out how to format an array. So many options.

[rwjblue]

@kellyselden

you're probably going to lose reliable git blames

I just can't agree here. Perhaps you will loose reliable first level git blame, but as I mentioned that really isn't enough anyways.

[rwjblue]

@kellyselden

All your responses to me seem like you think I'm arguing against the value of linting and autofixing.

Nah, I know we agree much more than disagree here.🥇

Most likely I just value the overall reduction in both mental overhead when authoring code (no need to get format "just right") and bikeshedding that you get on teams when using 👋 some other tool 👋 to do stylistic formatting and it is therefore out of your control a little bit more.

[kellyselden]

first level git blame

What do you mean by first level? If an array/destructure has lots of activity, prettier will continually collapse and expand the line (seen in real life), adding a new git blame indirection each time.

[rwjblue]

@kellyselden

What do you mean by first level?

I mean doing literally a single git blame without attempting to look at history prior to the most recent change on the line (or without looking at what the change to the line actually was).

If an array/destructure has lots of activity, prettier will continually collapse and expand the line (seen in real life), adding a new git blame indirection each time.

Yep, definitely happens (I've seen it too!), though this seems like an exception and not the normal experience. It will only happen when a given statement is right on the cusp of being too long for the one liner.

Ultimately, even if you conclude that this line wrapping (or unwrapping) is not ideal (which I'm not really sure I agree with TBH) the benefits to the overall codebase (and team productivity) far outweigh that cost.

[abhilashlr]

🎉 Nice to have this adopted as part of the ember's ecosystem and while creating new ember apps, this becomes the perfect way to start. However, I had a question on the default blueprint files that are generated using the ember generate command:

The default ember-source blueprints that generate test cases would need to be altered. For example, the default generated unit test case follows:

module('<name>, function(hooks) {

Now, Prettier would add an error on function(hooks) as it expects function (hooks). I tried to search for a config that can alter this but couldn't find one or maybe my search was bad 😄 .
As an example, we introduced this RFC approach in upgrade-guide ember app and I had to update the test cases with function (hooks) instead of function(hooks).

I believe the codemod you are suggesting (ember-cli-prettier-codemod) should handle this as well as the initial migration?

[Gaurav0]

@abhilashlr There is no configuration option for this. For why, read https://prettier.io/docs/en/option-philosophy.html

Yes, I think it will be necessary to audit our blueprints for prettier errors.

[abhilashlr]

@abhilashlr There is no configuration option for this. For why, read https://prettier.io/docs/en/option-philosophy.html

Yep, I ended up with the same understanding, as the one you mentioned. However, this might tend to be huge changes considering large applications that have numerous files.

Yes, I think it will be necessary to audit our blueprints for prettier errors.

Yep, this could also cascade across addons that generate tests, I believe? Although I don't have a concrete number of addons that currently generate test cases, if they do, they also need to update their blueprints.

Few quick addons that I came across:

1. https://github.com/typed-ember/ember-cli-typescript-blueprints/blob/master/blueprints/component-test/qunit-rfc-232-files/__root__/__testType__/__path__/__test__.ts
2. https://github.com/minutebase/ember-can/blob/master/blueprints/ability-test/index.js

[ro0gr]

I think it will be necessary to audit our blueprints for prettier errors.

I'm not sure if we should do it, while the code generated by blueprints is syntactically valid.

Technically, apps may have some extra code style rules defined in their own eslint config or something. In this case blueprint output would also conflict with the certain project's codestyle. So in my opinion, this seems to be a pre-existing issue.

I think this kind of issue may/should be solved via running(preferably automatically) a lint --fix after the blueprint output.

[abhilashlr]

I think it will be necessary to audit our blueprints for prettier errors.

I'm not sure if we should do it, while the code generated by blueprints is syntactically valid.

Technically, apps may have some extra code style rules defined in their own eslint config or something. In this case blueprint output would also conflict with the certain project's codestyle. So in my opinion, this seems to be a pre-existing issue.

I think this kind of issue may/should be solved via running(preferably automatically) a lint --fix after the blueprint output.

@ro0gr Let's say, we create a new ember app using ember new <sample-app>, and then use ember g component <new-component> commands. That would be showing errors without even a single line of ESLint config in place, isn't it?

[ro0gr]

That would be showing errors

@abhilashlr if ember-cli automatically runs lint --fix afterwards, then no, it should not. But I suspect, this functionality might need to be considered separately, though it still feels better than changing the blueprints manually imo.

[jgwhite]

We’ve been using Prettier for JS-files only on a green field codebase at work for about a year now and our experience has been overwhelmingly positive. I’m really excited about this RFC and support it whole-heartedly.

Regarding Prettier and HBS templates — this is definitely still ongoing but great progress has been made. When we are collectively happy with Prettier’s handling of HBS templates then we can push for the Prettier team to mark HBS templates as officially supported.

[rwjblue]

@abhilashlr

However, I had a question on the default blueprint files that are generated using the ember generate command. The default ember-source blueprints that generate test cases would need to be altered.

Ya, definitely a good call out here. The prose in the RFC under the header "Blueprint Changes" is intending to make it clear that all addon provided blueprints (including ember-source, ember-data, ember-cli owned ones) should be updated to satisfy the (new) default linting requirements.

Here is a copy of that prose (for easier reference):

Blueprint Changes

In general, it is recommended that all blueprints provided by addons should
satisfy the default linting configuration of a new Ember application. As such
the blueprints provided by ember-cli, ember-source, and ember-data will
be updated to ensure that they satisfy these new linting rules requirements.

[rwjblue]

@ro0gr

I think it will be necessary to audit our blueprints for prettier errors.

I'm not sure if we should do it, while the code generated by blueprints is syntactically valid.

Technically, apps may have some extra code style rules defined in their own eslint config or something. In this case blueprint output would also conflict with the certain project's codestyle. So in my opinion, this seems to be a pre-existing issue.

I think this kind of issue may/should be solved via running(preferably automatically) a lint --fix after the blueprint output.

Ya, this is a good idea, thank you!

While I still think blueprints should be updated to conform to the default app/addon blueprint linting requirements (most addons that own blueprint would consider it a bug to have their blueprints fail linting immediately upon generation), running yarn lint:fix (when it is defined in the package.json) after running a blueprint (for ember g, ember init, and ember-cli-update) is still a good idea. I've pushed that tweak in 6ceceba.

[patocallaghan]

I'm a big fan of this change. I'll generally add Prettier to a new app or addon anyway 😄 I mightn't agree with all of Prettier's formatting decisions but the fact that I don't have to think about it is a big win for me.

We introduced Prettier on a large repo about 2 years ago and while the initial rollout was a bit bumpy as not everyone had their editors set up correctly, the developer feedback was overwhelmingly positive. At time we found that there was a ~60% reduction in lint errors on CI (~2K errors less to the previous month) as all stylistic lints were now being handled by Prettier.

[luketheobscure]

Big fan of this change. I've seen firsthand how much PR chatter and code churn is eliminated when everyone is on prettier. In addition, getting eslint + prettier working together can be a bit confusing at first, so getting it all set up as the default is helpful.

I also agree with the sentiment here that prettier for hbs isn't quite ready to be the default. It's darn close, but there's at least two issues I know of that would be blockers for some people.

[mehulkar]

Why wouldn't you want to share the cache with your teammates?

[jrjohnson]

The thought of checking in a cache file gives me the heebie jeebies. I'm fine with just adding it to .gitignore myself though if I'm alone in that.

[rwjblue]

@mehulkar - This file would necessarily be changed by every commit that touches a .js file. That would make it very prone to merge conflicts, and AFAICT (did a quick local test) eslint doesn't automatically resolve these merge conflicts for you. Checking the file in would result in large amounts of pain, whereas not checking it in "just works" (and after that first linting run locally you get a nice speed boost).

[wycats]

We discussed this RFC at today's framework meeting and believe it's ready to merge. Thanks to everyone for the great conversation. I'm personally really looking forward to this change!