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

trailingComma: 'all' does not work when using rest operator in destructuring assignment #3883

Closed
aquilae opened this issue Feb 3, 2018 · 4 comments
Labels
lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker.

Comments

@aquilae
Copy link

aquilae commented Feb 3, 2018

Prettier 1.10.2
Playground link

--print-width 30
--trailing-comma all

Input:

function test(arg) {
	const { prop1, prop2, ...otherProps } = arg;
}

Output:

function test(arg) {
  const {
    prop1,
    prop2,
    ...otherProps
  } = arg;
}

Expected behavior:
Should add trailing comma to ...otherProps.
Saw some discussion about it here, but the fact is it's definitely inconsistent as it is as removing the rest operator results in comma being there.

@j-f1 j-f1 added type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker. lang:javascript Issues affecting JS labels Feb 3, 2018
@j-f1
Copy link
Member

j-f1 commented Feb 3, 2018

This is intentional: tc39/proposal-object-rest-spread#47. The trailing comma is forbidden after the rest property.

@j-f1 j-f1 closed this as completed Feb 3, 2018
@aquilae
Copy link
Author

aquilae commented Feb 3, 2018

Isn't the whole point of all option is to add commas everywhere even if it's not supported by spec?
If that's not the case then the difference between all and es5 seems pretty vague.
And what's the difference between this and function parameters comma which is, as I understand, forbidden as well?

@j-f1
Copy link
Member

j-f1 commented Feb 4, 2018

Isn't the whole point of all option is to add commas everywhere even if it's not supported by spec?

No, because we strive to always output valid JS.

If that's not the case then the difference between all and es5 seems pretty vague.

es5 outputs commas where they’re allowed by the (9-year-old) ECMAScript 5 spec, whereas all outputs commas where they’re allowed by the current spec, including in syntax proposals.

And what's the difference between this and function parameters comma which is, as I understand, forbidden as well?

Not in the latest edition of ECMAScript.

@aquilae
Copy link
Author

aquilae commented Feb 4, 2018

OK, I see, thanks for extensive answer.
Guess we'll have to tweak our lint config instead.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
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. type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker.
Projects
None yet
Development

No branches or pull requests

2 participants