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.

Sign up for GitHub

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

Jump to bottom

Set trailingComma default value to es5 #6963

Merged
merged 13 commits into from Nov 18, 2019
Merged

Set trailingComma default value to es5 #6963

merged 13 commits into from Nov 18, 2019
+2,923 −2,922

Conversation

fisker
Copy link
Sponsor Member

@fisker fisker commented Nov 15, 2019

#6910

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

✨Try the playground for this PR✨

@fisker fisker changed the title Set trailingComma default value to es5 [WIP] Set trailingComma default value to es5 Nov 15, 2019
alexander-akait
alexander-akait reviewed Nov 15, 2019
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @prettier/core
Why don't use engines.node field from package.json for better 0CJS and keep this option as none by default?

@fisker
Copy link
Sponsor Member Author

fisker commented Nov 15, 2019 •
edited

@evilebottnawi come on, it's 2019, still some engines.node requires none?

@alexander-akait
Copy link
Member

@fisker no, but some people can write es3 code in theory can inserting trailing commas can break code, but if we don't support es3 we can set es5 by default, but for better 0CJS we can respect node version

@fisker fisker changed the title [WIP] Set trailingComma default value to es5 Set trailingComma default value to es5 Nov 15, 2019
@fisker
Copy link
Sponsor Member Author

fisker commented Nov 15, 2019

Self checked, didn't find any wrong result.

This is hard to review, but I think it's ready

@fisker fisker mentioned this pull request Nov 15, 2019
Merged
ExE-Boss
ExE-Boss reviewed Nov 15, 2019
View reviewed changes
src/language-js/options.js
{
value: "es5",
description:
"Trailing commas where valid in ES5 (objects, arrays, etc.)"
},
{ value: "none", description: "No trailing commas." },
Copy link
Contributor

@ExE-Boss ExE-Boss Nov 15, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6910 (comment)

I’d prefer if the order wasn’t changed, as that way, es5 is still implied to be between none and all.

Copy link
Sponsor Member Author

@fisker fisker Nov 15, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6910 (comment)

@ExE-Boss why? after changing default to es5, shouldn't we put it on the top?

fisker
fisker commented Nov 15, 2019
View reviewed changes
tests/angular_component_examples/jsfmt.spec.js
@@ -1,2 +1,2 @@
run_spec(__dirname, ["typescript"], { trailingComma: "none" });
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6910 (comment)

I change the test orders to help review, snap changes should only be options part

@fisker fisker mentioned this pull request Nov 15, 2019
Closed
4 tasks
@lydell
Copy link
Member

lydell commented Nov 16, 2019

@evilebottnawi We already turned down #6936 so adding automatic support for ES3 here won’t make sense. I think we should keep things simple. es5 is always the default.

lydell
lydell approved these changes Nov 16, 2019
View reviewed changes
Copy link
Member

@lydell lydell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s do this!

@lipis
Copy link
Member

lipis commented Nov 18, 2019

So merginnnnn?

@alexander-akait
Copy link
Member

Let's do it

@lipis lipis merged commit 791996d into prettier:next Nov 18, 2019
@fisker
Copy link
Sponsor Member Author

fisker commented Nov 18, 2019

👍

sosukesuzuki pushed a commit to sosukesuzuki/prettier that referenced this pull request Jan 2, 2020
@fisker @sosukesuzuki
Set trailingComma default value to es5 (prettier#6963)
9d71dc9 
* Set `trailingComma` default value to `es5`

* Update `options` documentation

* Update tests

* style

* fix js code style

* change options order

* update tests_integration snap

* disable es5 for __ng_interpolation

* fix angular tests

* fix scss test

* fix `css_trailing_comma` snap

* fix css_scss snapshot
@sosukesuzuki sosukesuzuki mentioned this pull request Jan 6, 2020
Closed
lipis added a commit that referenced this pull request Jan 8, 2020
@lipis
Merge branch 'next' of github.com:prettier/prettier into next
ac6c6fa 
* 'next' of github.com:prettier/prettier:
  Optimize some usage of `Array#filter` (#6996)
  Update `jest` to v24 (#6954)
  Replace `trim{Left,Right}` with `trim{Start,End}` (#6994)
  Set `trailingComma` default value to `es5` (#6963)
  Fix `new` usage for builtin objects (#6968)
  Replace `indexOf` with `includes` (#6967)
  fix: tests for empty type parameters in TS (#6960)
  Fix MDX html parsing errors (#6949)
  fix: issue #6813 (Zero-based lists are broken) (#6852)
  Style: use async functions (#6935)
  Disable trailingComma for Angular internal parser (#6912)
  Update `snapshot-diff` to v0.6.1 (#6955)
  Update build scripts to target Node.js 10 (#6908)
@kachkaev kachkaev mentioned this pull request Jan 9, 2020
Closed
@sosukesuzuki sosukesuzuki added this to the 2.0 milestone Jan 10, 2020
@kachkaev
Copy link
Member

kachkaev commented Jan 25, 2020 •
edited

@fisker I'm working on #6929 and am about to create changelog_unreleased/api/pr-7430.md as asked in #7417 (comment). By looking at your PR, I cannot find a corresponding entry to use it as an example. If I simply fail in my search, could you please share a link? Otherwise, could you please add a corresponding blogpost section for your change? I'll try writing mine is a similar style.

@kachkaev kachkaev mentioned this pull request Jan 25, 2020
Merged
4 tasks
@fisker fisker deleted the trailing-comma-es5 branch January 25, 2020 19:02
@fisker
Copy link
Sponsor Member Author

fisker commented Jan 25, 2020

@kachkaev I didn't wrote one, if it's required, hope someone can help me out. I'm really bad at docs and English :(

@kachkaev kachkaev mentioned this pull request Jan 26, 2020
Merged
2 tasks
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Apr 25, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lipis lipis lipis approved these changes

@lydell lydell lydell approved these changes

@ExE-Boss ExE-Boss ExE-Boss approved these changes

@alexander-akait alexander-akait alexander-akait approved these changes

@j-f1 j-f1 j-f1 approved these changes

Assignees
No one assigned
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@fisker @alexander-akait @lydell @lipis @kachkaev @ExE-Boss @j-f1 @sosukesuzuki