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

Create except-object exception to arrow-body-style rule, enforcing explicit return of object literals from arrow functions. #5936

Closed
duncanbeevers opened this issue Apr 22, 2016 · 37 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@duncanbeevers
Copy link
Contributor

duncanbeevers commented Apr 22, 2016

The arrow-body-style rule enforces braceless function bodies when the function consists of a single return statement.

When constructing arrow functions which return object literals, the return value must be wrapped in parentheses to force its evaluation as an expression.

The proposed new option enforces a block body with an explicit return value for object literals returned from arrow functions.

Valid

var foo = () => { return; { bar: 0 }; };
var foo = () => 0;

Invalid

var foo = () => ({ bar: 0 });
@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Apr 22, 2016
duncanbeevers added a commit to duncanbeevers/eslint that referenced this issue Apr 22, 2016
…t#5936)

Blockless arrow functions are awesome, but when returning an object literal it's necessary to enclose the return value in parentheses. This rule extension enforces blockless arrow functions excluding cases where the arrow function returns an object literal.
@alberto alberto added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 22, 2016
@nzakas
Copy link
Member

nzakas commented Apr 23, 2016

I'm not sure I'm understanding this correctly. You're proposing to add an option that requires the use of return if you want to return an object literal, is that correct?

@duncanbeevers
Copy link
Contributor Author

Yes, exactly.

@BYK
Copy link
Member

BYK commented Apr 23, 2016

I think this is a fair request.

duncanbeevers added a commit to duncanbeevers/eslint that referenced this issue Apr 23, 2016
…t#5936)

Blockless arrow functions are awesome, but when returning an object literal it's necessary to enclose the return value in parentheses. This rule extension enforces blockless arrow functions excluding cases where the arrow function returns an object literal.
@nzakas
Copy link
Member

nzakas commented Apr 25, 2016

@BYK are you championing this change?

My feedback:

  • It's unclear to me if this is really a third option, equal to "always" and "as-needed". It seems like it could actually apply in addition to " as-needed". This needs some discussion.
  • I don't think "except-object" is a good name. We generally try to stay away from using "except" because it's unclear. We tend to use "allow", " skip", etc., words that have clearer meaning.

@duncanbeevers
Copy link
Contributor Author

duncanbeevers commented Apr 25, 2016

Yes, the naming for this is a little awkward. "except-object" is a refinement of "as-needed", not a distinct third option.

Conceivably, this could live as its own eslint plugin (parenthesized-object-literals or something) but in practice arrow functions are the only place I've seen them appear, as well as the only place I want to enforce this.

@BYK
Copy link
Member

BYK commented Apr 25, 2016

Don't want to over commit so holding on for championing for now in case someone else can do it. If we can't find anyone else in a week I'll take it on. Sounds good?

Also agree that this needs more discussion and consideration before moving forward with the code.

@BYK
Copy link
Member

BYK commented May 2, 2016

@eslint/eslint-team - anyone else interested in this rule? Either championing or contributing to the discussion?

@mikesherov
Copy link
Contributor

I vote that this should be a third option as its not truly needed.

@nzakas
Copy link
Member

nzakas commented May 4, 2016

@BYK it's been a week, are you championing? :)

Also, agree with @mikesherov that this would be a third option.

@BYK
Copy link
Member

BYK commented May 4, 2016

@nzakas - okay you got me, championing.

@nzakas
Copy link
Member

nzakas commented May 4, 2016

Okay, we just need three 👍s to move forward.

I'll give you one: 👍

@mikesherov
Copy link
Contributor

👍

@alberto
Copy link
Member

alberto commented May 4, 2016

👍 @BYK I could also take this

@BYK
Copy link
Member

BYK commented May 4, 2016

@alberto I'm just getting back to speed so if you have free time go for it. Otherwise I'll get to this eventually :)

@alberto alberto added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 4, 2016
@alberto alberto self-assigned this May 4, 2016
@alberto
Copy link
Member

alberto commented May 4, 2016

Cool

@alberto alberto added the needs bikeshedding Minor details about this change need to be discussed label May 4, 2016
@nzakas nzakas removed the needs bikeshedding Minor details about this change need to be discussed label May 5, 2016
@alberto
Copy link
Member

alberto commented May 6, 2016

I added the needs bikeshedding because I think we still need to settle on the option name (sorry for not being explicit). Is that tag appropriate for this?

@alberto
Copy link
Member

alberto commented May 12, 2016

I'm having trouble finding a name for a standalone option :( Any ideas?

@duncanbeevers
Copy link
Contributor Author

duncanbeevers commented May 12, 2016

What do you think of a third argument which refines the existing "as-needed" setting?

"arrow-body-style": [2, "as-needed", "skip-object-literal"]

@nzakas
Copy link
Member

nzakas commented May 15, 2016

We could do allowObjectLiteralBody with a default value of true and let people switch to false to force the use of curlies.

@BYK as champion, can you drive this discussion to a conclusion?

@duncanbeevers
Copy link
Contributor Author

@BYK @alberto

What do you think of using an extension similar to the avoidQuotes option used for the object-shorthand rule?

"arrow-body-style": [2, "as-needed", { avoidParens: true }]

@nzakas
Copy link
Member

nzakas commented May 16, 2016

I don't think avoidParens is specific enough. I would expect that option to also flag a => (B)

@BYK
Copy link
Member

BYK commented May 17, 2016

I'm +1 to allowObjectLiteralBody: true suggestion for now. If we get 2 more 👍s I'd say we go with that.

@alberto
Copy link
Member

alberto commented May 17, 2016

I'm sorry but I'm still not sure if you intend that as a new option or as a refinement for as-needed. @nzakas @BYK

@platinumazure platinumazure added the needs bikeshedding Minor details about this change need to be discussed label May 17, 2016
@platinumazure
Copy link
Member

Added "needs bikeshedding" label since we're still hashing a few things out.

@BYK
Copy link
Member

BYK commented May 17, 2016

I think this should be an option for as-needed mode.

@alberto
Copy link
Member

alberto commented May 17, 2016

Thanks, that makes more sense to me. I was bit confused by the previous conversation. Should we use requireForObjectLiteral instead?. My concern is allow doesn't convey not using braces is discouraged in that case.

@BYK
Copy link
Member

BYK commented May 18, 2016

@alberto - I'm not really sure actually. requireForObjectLiteral implies "require 'arrow function body' for object literal" which is an option to make the rule more strict. AFAIK we tend to have options that relax the original rule and allowObjectLiteralBody fits into that.

What do you think?

@alberto
Copy link
Member

alberto commented May 18, 2016

Ok, I think I see your point.

@ilyavolodin
Copy link
Member

It looks like there are two PRs for this issue: #5934 and #6216 Should we close one of them?

@BYK
Copy link
Member

BYK commented Jun 3, 2016

I'd close #5934 since it was created before the issue was accepted (hence before all these discussions). That said since @duncanbeevers is a new collaborator, there's a case to get his first patch in too. What do you think @ilyavolodin and @alberto.

@mikesherov
Copy link
Contributor

I think we should favor new contributions here.

@ilyavolodin
Copy link
Member

Agree with @mikesherov @alberto Would you mind guiding @duncanbeevers with the necessary updates to his PR?

@alberto
Copy link
Member

alberto commented Jun 4, 2016

oh, sorry, I didn't see that PR since it wasn't linked to the issue.

@duncanbeevers are you still interested in implementing this?

@duncanbeevers
Copy link
Contributor Author

I appreciate being favored for contribution, but I @alberto's #6216 is already done and ready for inclusion. I can't imagine implementing it any differently than they did, so I'd prefer #6216 to #5934

@BYK
Copy link
Member

BYK commented Jun 9, 2016

@duncanbeevers - thanks! Alright then, I closed #5934 and I think #6216 is ready to merge.

@alberto
Copy link
Member

alberto commented Jun 9, 2016

ok, @duncanbeevers. Thanks for your contribution anyway!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

8 participants