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 arguments #21984

Open
JoshuaKGoldberg opened this issue Feb 16, 2018 · 4 comments
Open

Allow trailing commas in type arguments #21984

JoshuaKGoldberg opened this issue Feb 16, 2018 · 4 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Feb 16, 2018

TypeScript Version: 2.7.1

Search Terms: trailing comma template literal types generic

Code

class FooClass<
	A,
	B,
	C,
> {
	a: A;
	b: B;
	c: C;
}

const instance = new FooClass<
	boolean,
	number,
	string, // [ts] Trailing comma not allowed.
	>();

Expected behavior:

No errors as of #16152's resolution.

Actual behavior:

Trailing comma not allowed.

Playground Link: http://www.typescriptlang.org/play/#src=class%20FooClass%3C%0D%0A%09A%2C%0D%0A%09B%2C%0D%0A%09C%2C%0D%0A%3E%20%7B%0D%0A%09a%3A%20A%3B%0D%0A%09b%3A%20B%3B%0D%0A%09c%3A%20C%3B%0D%0A%7D%0D%0A%0D%0Aconst%20instance%20%3D%20new%20FooClass%3C%0D%0A%09boolean%2C%0D%0A%09number%2C%0D%0A%09string%2C%20%2F%2F%20%5Bts%5D%20Trailing%20comma%20not%20allowed.%0D%0A%09%3E()%3B

Related Issues:

#16152. @samal84 mentioned something like this but I couldn't parse what they meant without a code sample (is this it?). I'll take I can, if this is approved, take a stab at removing this error too.

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Help Wanted You can do this Good First Issue Well scoped, documented and has the green light labels Feb 16, 2018
@DanielRosenwasser DanielRosenwasser added this to the Future milestone Feb 16, 2018
alangpierce added a commit to alangpierce/assemblyscript that referenced this issue May 6, 2018
Trailing commas are allowed in these situations in JS/TS:
* Array literals
* Argument lists
* Parameter lists
* Enums
* Type parameter lists
* Type argument lists (Technically filed as microsoft/TypeScript#21984 )

They're also allowed in these cases not (yet) supported by AssemblyScript:
* Object literals
* Array destructures
* Object destructures

All of these cases had similar-looking code, which needed to be tweaked to
handle the possibility of a comma before the close-delimiter. Type arguments
were a little different because they take a backtracking approach.

I also fixed a missing case in the AST printer: `export function` wasn't being
printed right.
alangpierce added a commit to alangpierce/assemblyscript that referenced this issue May 6, 2018
Trailing commas are allowed in these situations in JS/TS:
* Array literals
* Argument lists
* Parameter lists
* Enums
* Type parameter lists
* Type argument lists (Technically filed as microsoft/TypeScript#21984 )

They're also allowed in these cases not (yet) supported by AssemblyScript:
* Object literals
* Array destructures
* Object destructures

All of these cases had similar-looking code, which needed to be tweaked to
handle the possibility of a comma before the close-delimiter. Type arguments
were a little different because they take a backtracking approach.

I also fixed a missing case in the AST printer: `export function` wasn't being
printed right.
@mhegazy mhegazy added Suggestion An idea for TypeScript and removed Bug A bug in TypeScript labels Jul 20, 2018
@mhegazy mhegazy added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed Help Wanted You can do this labels Jul 23, 2018
@mhegazy mhegazy removed this from the Future milestone Jul 23, 2018
@mhegazy mhegazy removed the Good First Issue Well scoped, documented and has the green light label Jul 24, 2018
@brainkim
Copy link

brainkim commented Aug 1, 2018

Copying another code example into this issue cuz @mhegazy says it’s related and my issue was closed:

interface  Pizza<T> {
  pizza: T;
}

type PizzaBox = Partial<Pizza<string,>>;

also incorrectly throws a Trailing comma not allowed error.

@bensaufley
Copy link

In response to #25939:

If we have more community interested, we'll reconsider it.

How can I and/or the community demonstrate interest?

I think "looks like an error" / "looks pretty gross" are pretty subjective and contrary to the widely-adopted Airbnb styles, which prefer trailing commas on multiline lists, etc—to be honest, I used to be opposed to it but since we've adopted the Airbnb styles, they've grown on me, especially in that they can make Git diffing much more readable.

Disallowing them in this particular case ends up having the effect that trailing commas can be used everywhere but one place in our code, and also ends up putting our linter config at odds with ts (or makes us disable the linter for this style, which means we can't be consistent elsewhere).

I entirely understand that there are complications that can arise from implementing this, including the "what if" listed, but I think it's worth reconsidering.

@alangpierce
Copy link
Contributor

My mental modal of trailing commas is that trailing commas are now allowed in all cases in JS, and I think that's a lot simpler than having to remember which syntax cases support them and which don't. Previously, the language had some opinions on when they should and should not be used, but now I believe they're consistently allowed in all comma-separated lists (arrays, objects, destructures, imports, exports, parameters, and arguments, though technically not after rest elements), and it's up to the community and the tools to decide if and when to use them. The benefit (described at https://github.com/tc39/proposal-trailing-function-commas ) is relatively minor, and it can be a source of inconsistency, but with tools like Prettier, you get the benefit for free without having to think about it, so I personally have a slight preference for trailing commas everywhere so that you get slightly cleaner diffs.

I usually assume that TypeScript is consistent with JavaScript in its design decisions (and it usually is), so I was surprised to learn that trailing commas were mysteriously disallowed in this one case. Obviously it's not going to be a huge roadblock for anyone, and multiline type argument lists are super-rare anyway, but I think consistent rules mean a little less cognitive overhead.

Put another way, I think the two most reasonable coding styles are "never use trailing commas ever" and "always use trailing commas for multiline lists". That second one is now possible in JS as of ES2017, but it's still not possible in TS due to this one disallowed case.

@dsherret
Copy link
Contributor

dsherret commented Mar 23, 2020

Perhaps this title should be updated to "Allow trailing commas in type arguments" since this seems to be the canonical issue for this? This seems to apply to all type arguments (but not type parameters).

@JoshuaKGoldberg JoshuaKGoldberg changed the title Allow trailing commas in new expression type argument lists Allow trailing commas in type arguments Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants