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

Allow trailing commas in type parameter/argument lists #16152

Closed
DanielRosenwasser opened this issue May 30, 2017 · 10 comments
Closed

Allow trailing commas in type parameter/argument lists #16152

DanielRosenwasser opened this issue May 30, 2017 · 10 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

Discussion from prettier/prettier#1820

Consistency makes sense to me here, but do we feel a trailing comma in a type param/arg list is a smell for some typo?

@Draqir
Copy link

Draqir commented May 30, 2017

I think the whole idea of allowing trailing commas is introducing more confusion then it helps. Why:

  • Inconsistency Trailing commas in JSON (JavaScript Object Notation) is an error. Trailing commas in object literals in JavaScript has been allowed since ES5.
  • Special rules One needs to know the special rules of trailing commas, they're allowed in functions in ES2017, but not after a rest parameter nor as a single argument to a function.
  • Ugly there's a reason why Fortran 77 (created in 1969) only allows compilation with trailing commas with the flag -fugly-comma

To me, the only times I've had a trailing comma is because of a typo, I think they should be allowed but under some flag like "fuly-comma": true and disabled by default.

@vjeux
Copy link

vjeux commented Jun 2, 2017

Another place where typescript doesn't support trailing commas: prettier/prettier#1858

@vjeux
Copy link

vjeux commented Jun 2, 2017

Note that trailing commas for function calls and arguments is now in stage 3 at tc39 and node 8 that was just released this week supports them.

https://github.com/tc39/proposal-trailing-function-commas/blob/master/README.md

@mhegazy
Copy link
Contributor

mhegazy commented Jun 2, 2017

Note that trailing commas for function calls and arguments is now in stage 3 at tc39 and node 8 that was just released this week supports them.

trailing commas in function calls and parameter lists are already supported in TS since typescript@2.0.

the question here is whether to extend that to type declarations or not.

@huan
Copy link

huan commented Sep 1, 2017

I vote for allowing it because the trailing commas almost allowed everywhere, so if the type declarations do not, it will smell very strange for me.

Like the follow TypeScript will throw [ts] Trailing comma not allowed error, which I would like to keep the trailing comma for it very much.

type Command = 'align' | 'embedding'

interface Args {
  command: [
    Command,
    string,
  ]
}

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Oct 30, 2017
@JoshuaKGoldberg
Copy link
Contributor

Now that there are generic parameter defaults, there are times when using them in combination with a typical 120 line cap in linting (the TSLint default at least) makes it necessary to break across lines.

type TemplateBinding =
    | AppleTemplateBinding
    | BananaTemplateBinding
    | CarrotTemplateNode
;

type ModelNode =
    | AppleModelNode
    | BananaModelNode
    | CarrotModelNode
;

export const createSourceNodeFromTemplateBinding = <
    TTemplateBinding extends TemplateBinding = TemplateBinding,
    TSourceNode extends SourceNode = SourceNode
>(templateBinding: TTemplateBinding) => {
    /* ... */
};

Strong +1 to allowing them.

@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Jan 5, 2018
@mhegazy mhegazy added this to the TypeScript 2.7 milestone Jan 5, 2018
@samal-rasmussen
Copy link

I just ran into this "[ts] Trailing comma not allowed." error on a type. It was surprising, as I though Typescript supported trailing commas. I've now learned that this is only supported in parameter lists and object literals. I still find this surprising as this seems to be inconsistent. Either support trailing commas or don't. Doing it in some places and not in others is inconsistent and leads to surprises while writing Typescript.

@JoshuaKGoldberg
Copy link
Contributor

@samal84 can you file a new issue with sample code? If there's another unexpected place, I'd be interested in trying to fix that too.

@samal-rasmussen
Copy link

My situation was basically the same as the one you demonstrated above with multiple generic parameters to a function. The example that mhegazy demonstrated above is also something that may come up in real world codebases.

I also have a code formatting rule that if there is more than one parameter then every parameter should be on its own line. This makes for much shorter lines and makes the codebase much more consistent in general. Unfortunately I cannot enforce this rule for generic parameters in Typescript it seems.

@whitneyit
Copy link
Contributor

In regards to @JoshuaKGoldberg's comment,

It is odd that leading pipes are allowed:

type TemplateBinding =
    | AppleTemplateBinding
    | BananaTemplateBinding
    | CarrotTemplateNode
    ;

Whilst trailing pipes are not:

type TemplateBinding =
    AppleTemplateBinding |
    BananaTemplateBinding |
    CarrotTemplateNode | // Error. Type expected.
    ;

I would of imagined that the same Error would apply for the first example. It seems that between the = and the |, throwing a Type expected. Error seems reasonable.

If trailing commas are allowed, does that mean that trailing pipes should be too?

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants