Skip to content
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

Enforce Trailing Commas by Default #68

Closed
btnwtn opened this issue Jan 10, 2017 · 53 comments
Closed

Enforce Trailing Commas by Default #68

btnwtn opened this issue Jan 10, 2017 · 53 comments
Labels
lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Milestone

Comments

@btnwtn
Copy link

btnwtn commented Jan 10, 2017

I propose that trailing commas should be enabled by default. IE8 is a non issue for most, and this will provide better diffing.

const arr = [
  'multi',
  'line'
]

should be formatted as:

const arr = [
  'multi',
  'line',
]

I would also suggest not enabling it for function arguments, considering that this is not in the official spec yet and won't be fully supported without a transpiler like babel. e.g.:

foo(
  arg1,
  arg2,
)

should be formatted as:

foo(
  arg1,
  arg2
)
@Pomax
Copy link

Pomax commented Jan 10, 2017

I'll represent the other camp: clean, readable code does not need the trailing commas, as commas are separators, not prefix/postfix operators, and there's nothing following that comma. Injecting commas in this case would be a reason for me to not want to use this formatter.

@leeoniya
Copy link

leeoniya commented Jan 10, 2017

arrays often have line-level similarity and for editors that have a "duplicate line" functionality, a trailing comma at the end can yield better ergonomics.

EDIT: but if this is simply for immutable output, then i'd agree with stripping trailing

/ $0.02

@jlongster
Copy link
Member

Right now we at least have the option, so you can turn it on. This is a question of if it should be the default. I'm leaning towards not, because it I get the sense that the majority (by a little margin) doesn't use them, but I can let this sit for a while. You can at least turn them on if you want them.

@bnjmnt4n
Copy link
Contributor

I think they should not be enabled by default, due to the trailing commas in function parameters as well. In fact, that should probably also be made configurable similar to recast:

trailingCommas: {
  array: true,
  object: true, 
  function: false 
}

@jamiebuilds
Copy link

I get the sense that the majority (by a little margin) doesn't use them

@jlongster That probably has a lot to do with trailing commas being invalid in a number of places which has since changed. I know Airbnb's code style thing changed afterwards @ljharb

@ljharb
Copy link

ljharb commented Jan 11, 2017

Trailing function commas recently hit stage 4, and haven't been published in the standard yet (which it will be this June - although "published" is irrelevant, only "hit master on github" matters).

The airbnb config requires trailing commas anywhere they're allowed to appear, to ensure consistency and to minimize git diffs. Whether it's the default or not is certainly your choice, but my hunch is that the majority of the airbnb config is going to be very close to what most users use, either now, or eventually.

@okonet
Copy link
Contributor

okonet commented Jan 11, 2017

I'm opposed to that. I have tried both styles and thus better ergonomics in the editor is a valid argument but this only matters until I Finish editing it. Same with the diff — if the algorithm can't handle it, algorithm should be improved and not the source code get altered to please the machine.

The formatting of the code should not be dictated by tools but by readability. Tools should follow what is good for humans and not for robots. A trailing comma has no semantic but represents a visual noise.

@ljharb
Copy link

ljharb commented Jan 11, 2017

Minimal git diffs are good for humans, and consistency is good for readability.

@vramana vramana added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Jan 11, 2017
@potch
Copy link

potch commented Jan 11, 2017

I expect formatted code to run in browsers without any additional processing, and would prefer any feature that breaks that not be on by default. This rules out trailing commas in functions.

@ljharb
Copy link

ljharb commented Jan 11, 2017

Technically that rules out all forms of trailing commas as well as any unquoted reserved words, or variable names with reserved words - either you're hand coding to ES3, you're breaking browsers, or you're using some additional processing ¯\_(ツ)_/¯

@mattbasta
Copy link

@potch trailing commas in lists and objects is supported by everything in recent history (IE9 was the last browser affected, and only in compatibility mode; IE8 was the last browser to wholesale shit the bed on trailing commas).

I think, obviously, trailing commas in arg lists is a bad idea for now. I don't think it's reasonable to make it a default until IE11 is dead and gone and ES2017 has settled in all of the evergreen browsers for a few years.

As for why I support trailing commas, I find it prevents many errors when copying and pasting. It's a quality of life thing. If you use the "shift lines" feature in your text editor, you'll know how much of a hassle it is when you've got a missing comma. In particular, if you always use a trailing comma, you'll never have a problem. This is in contrast to, say, leaving off semicolons where you need to remember a handful of rules for when you have to include them.

@potch
Copy link

potch commented Jan 11, 2017

@mattbasta yeah, can't say I super care strongly about arrays and object literals for trailing commas. It feels like a violation of principle of least surprise to see characters added like that, but I'd rather have consistent formatting in an easy-to-use tool than it hew exactly to my preferences.

Just no syntax errors please! I guess I'd be annoyed if I was intentionally writing ES3 to have it ES5ified, but that's a super niche case and the basic config covers it.

@btnwtn
Copy link
Author

btnwtn commented Jan 11, 2017

@potch how do trailing commas in an array or object break browsers? Are we targeting IE8 here? And if so... please reconsider.

@mythmon
Copy link

mythmon commented Jan 11, 2017

I hope that tools like prettier wouldn't be concerned about ES3 compatibility. I'd vote for trailing commas in multiline arrays and objects by default, and optionally for parameter lists. As well as making VCS diffs easier, it also makes re-ordering lines easier in an editor, as one doesn't have to worry about the presence or absence of the trailing comma.

@ljharb
Copy link

ljharb commented Jan 11, 2017

(for what it's worth, I agree all this should be configurable; I should be able to use prettier for hand-writing ES3, ES5, ES6, etc, as well as for code intended to be run through babel - we're just talking about the default here)

@luisherranz
Copy link

I agree. We should be able to use prettier for ES5 code as well, not only trough babel.

Maybe we need .prettierrc files for ease of use, so we can add things like @demoneaux said on each project:

trailingCommas: {
  array: true,
  object: true, 
  function: false 
}

@btnwtn
Copy link
Author

btnwtn commented Jan 13, 2017

I'm actually going to close this issue as I think it completely misses the point of this project. I'm now of the opinion that there should be no config options for how code is formatted. Go fmt works this way and people are fine with it.

@btnwtn btnwtn closed this as completed Jan 13, 2017
@ljharb
Copy link

ljharb commented Jan 13, 2017

¯\_(ツ)_/¯ that's a quick path to ensuring a lack of adoption.

@luisherranz
Copy link

I'm now of the opinion that there should be no config options for how code is formatted.

I'm going to miss prettier for ES5 projects but yeah, no config is definitively better if everyone ends up using it, which I hope.

@ljharb
Copy link

ljharb commented Jan 13, 2017

That'd be great - but it'll never happen without maximum configurability.

@btnwtn
Copy link
Author

btnwtn commented Jan 13, 2017 via email

@luisherranz
Copy link

That'd be great - but it'll never happen without maximum configurability.

Then fork it.

@ljharb
Copy link

ljharb commented Jan 13, 2017

That's certainly an option, as is continuing to use eslint as the majority of the ecosystem does. Forking, however, will not likely help achieve the goal of "gofmt for JS"

@btnwtn
Copy link
Author

btnwtn commented Jan 13, 2017 via email

@btnwtn
Copy link
Author

btnwtn commented Jan 13, 2017 via email

@prettier prettier unlocked this conversation Jan 23, 2018
@j-f1
Copy link
Member

j-f1 commented Jan 23, 2018

Reopening since the v2.0 issue has almost 100 comments. I don’t think there’s anything else to say here, but I’m reopening this to track our progress on v2.0.

@j-f1 j-f1 reopened this Jan 23, 2018
@j-f1 j-f1 added this to the Prettier 2.0 milestone Jan 23, 2018
@hsribei
Copy link

hsribei commented Aug 16, 2018

+1 Please default to trailing commas!

I hate the added diff lines and the little dance to edit the last line then append one after it.

But I hate even more the idea of adding a .prettierrc file and configure my zero config code formatter.

So I'm deferring to whatever default prettier chooses, but hoping we can collectively lobby for the saner default. 🙏

@kripod
Copy link

kripod commented Aug 16, 2018

I’m not sure why the es5 option is preferred over all. Node.js 8 LTS already supports every type of trailing commas available.

@ljharb
Copy link

ljharb commented Aug 17, 2018

fwiw, eslint still defaults to es5, because that's a safer target given a wide range of current browser support.

@j-f1
Copy link
Member

j-f1 commented Aug 17, 2018

The original rationale was — if I remember correctly — that we don’t want to break ES5 code that people have written by default. If you use arrow functions or destructuring, we’ll preserve them, but we won’t convert ES5 constructs to ES>5 constructs, however trailingComma: all does that.

@kripod
Copy link

kripod commented Aug 17, 2018

Thanks for the nice explanations!

@wujekbogdan
Copy link

@btnwtn
Totally agree. Reasons to add trailing commas:

  • It's easier to refactor the code. When you change the order of object's properties then you don't have to play with adding/removing commas
  • It's easier to add new properties. You don't have to worry about adding a coma after the last property because it's already there.

@ExE-Boss
Copy link
Contributor

I’m in favour of this because it makes git diff a lot cleaner.

@slikts
Copy link

slikts commented May 12, 2019

Trailing commas being off by default is one of the worst aspects of Prettier, since users tend to take it as a recommendation against a good language feature.

@Pomax
Copy link

Pomax commented May 13, 2019

To be clear: the power of prettier is "you don't care about whether it's right or wrong, all you care about is that it's correct javascript that won't error at runtime, and always looks the same no matter who wrote it, because we use prettier".

And the rest of is 100% irrelevant: you right click, say "format document", and hit save - if that, because precommit hooks and "run on close" are almost trivial things in today's world. So it's not something you should be placing any more value on than just being what it is: a "stop caring about this" technology that frees up your team.

People reading Prettier-formatted code in order to learn what good JS style is, is kind of a crazy notion: that's not what most people do, they learn absolutely horrible JS already from every online tutorials, and even from many projects that have their own code styleguide that is opinioned about what constituted "good javascript" when the only good javascript is javascript that runs as intended, without erroring out. The spec allows for a ton of syntax that some part of the world will hate, but isn't actually bad in the slightest. Prettier does not make that any worse, or any better honestly. It's orthogonal: you use it, because you want to stop having to care.

@slikts
Copy link

slikts commented May 13, 2019

Stripping out trailing commas does make practical difference in the editing workflow and in creating diff churn. Prettier doesn't fix invalid syntax, so appending a line to a list or moving the last line requires manually adding the comma that otherwise would have been there. Appending a line to a list requires editing the previously last line, which shows up as a highlight in diffs and hence code review, so reading the diff requires mentally filtering out the noisy line. Likewise, when looking up blame info, changes to commas need to be stepped through to find the actual author of a line.

@lydell
Copy link
Member

lydell commented May 13, 2019

trailingComma: es5 is most likely going to be the default in 2.0.

@slikts
Copy link

slikts commented May 13, 2019

Yay for catching up to ES5, but it should be all.

@lydell
Copy link
Member

lydell commented May 13, 2019

Yes, some day.

@lydell lydell modified the milestones: 2.0 (maybe/old/stretch), 1.20, 2.0 Nov 9, 2019
@lydell lydell added lang:javascript Issues affecting JS and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Nov 9, 2019
@sosukesuzuki
Copy link
Member

#6963 has been merged to next branch, can we close this issue?

@sosukesuzuki
Copy link
Member

/cc @prettier/core I'll close this issue for above reason. if there is any problems please reopen.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Apr 15, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

No branches or pull requests