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

disallow trailing function commas #787

Closed
ungoldman opened this Issue Feb 18, 2017 · 8 comments

Comments

5 participants
@ungoldman
Copy link
Member

commented Feb 18, 2017

I ran into a weird edge case where a trailing comma was introduced into a function call, like so:

foo(
  arg1,
  arg2,
)

The linter didn't catch this, and surprisingly the parser didn't catch this either (running in electron, node fork currently at 6.5.0) and it ran fine without throwing a syntax error on macOS, but then failed due to a syntax error on windows (same electron app, same version, theoretically exact same context other than OS).

This was a little surprising for a number of reasons (edit: found out after the fact this is a babel/ES2017 feature I was unaware of), but I think the only one relevant to standard is that trailing function commas are allowed with the current eslint config.

I learned that the comma-dangle rule's string option allows trailing function commas by default.

Trailing commas in function declarations and function calls are valid syntax since ECMAScript 2017; however, the string option does not check these situations for backwards compatibility.

functions is for function declarations and function calls. (e.g. (function(a,){ })(b,);)
functions is set to "ignore" by default for consistency with the string option.

ref: http://eslint.org/docs/rules/comma-dangle

So if standard should disallow trailing function commas, it needs to explicitly set the comma-dangle object option like so:

"comma-dangle": ["error", {
  "arrays": "never",
  "objects": "never",
  "imports": "never",
  "exports": "never",
  "functions": "never"
}]

If you'd like me to open an issue or PR on eslint-config-standard please let me know.

Thanks!

@feross

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

Yes, agree with you. We should prevent this for consistency.

@feross feross added this to the standard v10 milestone Feb 20, 2017

@feross feross added the enhancement label Feb 20, 2017

@feross

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

No ecosystem impact. This will be part of the standard v10 beta.

@feross feross closed this Mar 1, 2017

@ungoldman

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2017

🎉 yay! 🎉

feross added a commit to standard/eslint-config-standard that referenced this issue Mar 2, 2017

@tdd

This comment has been minimized.

Copy link

commented Mar 3, 2017

@feross considering how far back trailing commas have been allowed by language (for arrays and objs, it's been there since ES3 I believe), and that imports/exports are an automatic indication we're in ES2015+, therefore either transpiled or run by an engine that won't burp on it, the only reason I see for Standard forbidding dangling commas on arrays and objects is IE < 9.

Does Standard have an explicit goal to address legacy browsers that far back? I mean, dangling commas are a major benefit wrt versioning, as they allow for much better diffs. I get the function-args case, as this is only allowed in ES2017+ but the syntactic context has been around forever, so a case can be made for it. But the other four seem to keep us firmly in the past when there's no real benefit IMHO.

What do you think?

@tdd

This comment has been minimized.

Copy link

commented Mar 3, 2017

Oh and BTW, there's apparently traction, in other issues, for adjusting Standard preferences incrementally towards "runtimes supporting ES0215+" (see the prefer-const discussion and Node 0.x being EOL'd now, for instance), so relaxing dangling commas on objects, arrays, imports and exports seems consistent with that.

@dcousens

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

I don't think browser compatibility is a consideration at all [for standard].
This issue was merely about consistency with the existing dangling comma ruling; of which, a decision was made a while ago, with which I don't think relaxing is an option.

@tdd

This comment has been minimized.

Copy link

commented Mar 3, 2017

Hey Daniel, thanks for pitching in.

Relaxing rules has virtually zero ecosystem impact, unlike tightening them. I totally get that Standard cannot afford to seem mercurial on its decisions, but that shouldn't mean every decision has to be etched in stone, as some of them are inherently tied to the state of the runtimes / best practices of the day, and these evolve. Quite fast, too.

I'm unaware of the rationale that originally underpinned the "no dangling commas in arrays/objects" for Standard. I know that for ESLint, this was "IE < 9", whose JScript runtime would break on them. Other than that, dangling commas have been considered useful in every lang, incl. JS, precisely for versioning purposes.

So perhaps a careful reconsideration, from time to time, of long-established rule decisions might be a good idea to keep Standard 100% relevant and in phase with latest best practices…?

Cheers,

@flockonus

This comment has been minimized.

Copy link

commented Mar 7, 2017

I understand this ship has sailed, but since #586 is closed out, this behaviour of not allowing trailing commas feels super weird to me.
I'm all in favour of no semis, but not trailing commas are just convenient

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.