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

Set trailingcomma's to ES5 #60

Closed
wants to merge 1 commit into from
Closed

Set trailingcomma's to ES5 #60

wants to merge 1 commit into from

Conversation

ramonakira
Copy link
Member

Since prettier changed their default setting to all instead of es5, we have to specify es5 now.

See: https://prettier.io/docs/en/options.html#trailing-commas

@ramonakira ramonakira requested a review from a user February 26, 2024 09:54
@jaap3
Copy link
Member

jaap3 commented Feb 26, 2024

This happened a while ago, and some projects have already been updated using this setting i.e.: https://github.com/leukeleu/at-frontend/commit/0c889a5535abaedd239eabea23e7c7ebf7794f2f

Browser support for trailing commas everywhere seems to be generally available since ~2017 (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Trailing_commas#browser_compatibility).

I'm going with a -1 on this (just run prettier on projects to autofix the formatting)

@ramonakira
Copy link
Member Author

We actually went through this before, where I proposed setting it to ES5 and at the time the rule was removed from our config because it was the default. See: #36 and now it comes back to bite us in the ass 😛

@jaap3
Copy link
Member

jaap3 commented Feb 26, 2024

I see, however that change only made it to version 1.4 (https://github.com/leukeleu/prettier-config/releases/tag/v1.4.0). Before that, the config specified "all" and after that prettier was updated to default to "all".

@ghost
Copy link

ghost commented Feb 26, 2024

Is going with all really going to give us trouble? I like trailing commas 😄

@jaap3
Copy link
Member

jaap3 commented Feb 26, 2024

I think we should just keep "all", it's been the default for this config in all versions except one. So changing it now does more harm than good imho.

@ramonakira ramonakira closed this Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants