Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

No trailing commas for tuple types #2418

Closed
MichaReiser opened this issue Apr 13, 2022 · 6 comments
Closed

No trailing commas for tuple types #2418

MichaReiser opened this issue Apr 13, 2022 · 6 comments
Labels
A-Formatter Area: formatter

Comments

@MichaReiser
Copy link
Contributor

MichaReiser commented Apr 13, 2022

Rome formatter inserts a trailing comma for tuple types, Prettier doesn't

Input

type A = [
  AAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBBBBBBBBBBBDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD,
  String,
  Number
 ];

Prettier

type A = [
  AAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBBBBBBBBBBBDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD,
  String,
  Number
];

Rome

type A = [
  AAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBBBBBBBBBBBDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD,
  String,
  Number,
];

Expected

No trailing comma after Number

@ematipico
Copy link
Contributor

ematipico commented Apr 13, 2022

I don't think we should adhere to this.

So sorry guys, but I am adamant on this one, count me out 😆
Of course, if the majority of people think we should adhere, I am all in.

Trailing commas are really useful (I hated them but now I love them) when we want to add additional members at the end of the list. By duplicating the last line, the list is not broken and we can add stuff without the worry of the autocomplete not working.

Plus, it's consistent with the rest of the formatting (where trailing commas are allowed)

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Apr 13, 2022

Trailing commas are really useful (I hated them but now I love them) when we want to add additional members at the end of the list. By duplicating the last line, the list is not broken and we can add stuff without the worry of the autocomplete not working.

Plus, it's consistent with the rest of the formatting (where trailing commas are allowed)

Prettier's trailingComma defaults to es5 which doesn't insert any trailing commas. Changing the option to all makes Prettier insert the semicolon too.

The Option's documentation explains why they aren't adding a trailing comma by default:

Trailing commas wherever possible (including function parameters and calls). To run, JavaScript code formatted this way needs an engine that supports ES2017 (Node.js 8+ or a modern browser) or downlevel compilation. This also enables trailing commas in type parameters in TypeScript (supported since TypeScript 2.7 released in January 2018).

So the issue is that TypeScript didn't support trailing commas until the 2.7 release. I think we can put this back on hold until we made a decision if Rome should support a trailingComma option or not.

@MichaReiser MichaReiser added needs triage and removed good first issue Good for newcomers labels Apr 13, 2022
@ematipico
Copy link
Contributor

@NicholasLYang @yassere do you mind weight in and share your thoughts? Given the history shard by Micha and the fact that language targets are a non-goal for us, what's your take?

@yassere
Copy link
Contributor

yassere commented Apr 28, 2022

language targets are a non-goal for us

What do you mean by this?

If we're choosing not to support a trailingComma option at all, then I think it's reasonable to add trailing commas wherever possible. They're widely enough supported.

If we find that not including a trailingComma option is causing lots of migration friction for existing Prettier users, that's something we can revisit later if it seems to be an actual problem rather than a hypothetical one.

@ematipico
Copy link
Contributor

language targets are a non-goal for us

What do you mean by this?

If we're choosing not to support a trailingComma option at all, then I think it's reasonable to add trailing commas wherever possible. They're widely enough supported.

If we find that not including a trailingComma option is causing lots of migration friction for existing Prettier users, that's something we can revisit later if it seems to be an actual problem rather than a hypothetical one.

We don't target versions of the languages that are old.

For example es3 or old Node.js 8, as quoted by Micha.

@ematipico
Copy link
Contributor

As for now, target version is a non-goal for us and as we decided to not support trailingComma option, we won't implement this issue for the time being. If this causes frictions for adoptions, we can re-open and implement it.

@ematipico ematipico moved this to Done in Rome 2022 May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter
Projects
Status: Done
Development

No branches or pull requests

3 participants