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

Update: Add never option to arrow-body-style #6317

Closed
ajhyndman opened this issue Jun 3, 2016 · 14 comments
Closed

Update: Add never option to arrow-body-style #6317

ajhyndman opened this issue Jun 3, 2016 · 14 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

@ajhyndman
Copy link
Contributor

Version: 2.10.0

Arrow functions that return object literals can look very similar to arrow functions with brace bodies. Some syntactic ambiguity can be avoided by disallowing block-style arrow functions in favour of ES5 function expressions.

Outcome

The following patterns are considered problems:

/*eslint arrow-body-style: ["error", "never"]*/
/*eslint-env es6*/

let foo = () => {
    return 0;
};

let foo = (retv, name) => {
    retv[name] = true;
    return retv;
};

The following patterns are not considered problems:

/*eslint arrow-body-style: ["error", "never"]*/
/*eslint-env es6*/

let foo = () => 0;

let foo = () => ({ key: 0 });
@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jun 3, 2016
ajhyndman added a commit to ajhyndman/eslint that referenced this issue Jun 3, 2016
Arrow functions that return object literals can look very similar to arrow functions with brace bodies.  Some syntactic ambiguity can be avoided by disallowing block-style arrow functions in favour of ES5 function expressions.

**Outcome**

The following patterns are considered problems:
```
/*eslint arrow-body-style: ["error", "never"]*/
/*eslint-env es6*/

let foo = () => {
    return 0;
};

let foo = (retv, name) => {
    retv[name] = true;
    return retv;
};
```

The following patterns are not considered problems:
```
/*eslint arrow-body-style: ["error", "never"]*/
/*eslint-env es6*/

let foo = () => 0;

let foo = () => ({ key: 0 });
```
ajhyndman added a commit to ajhyndman/eslint that referenced this issue Jun 3, 2016
Arrow functions that return object literals can look very similar to arrow functions with brace bodies.  Some syntactic ambiguity can be avoided by disallowing block-style arrow functions in favour of ES5 function expressions.

**Outcome**

The following patterns are considered problems:
```
/*eslint arrow-body-style: ["error", "never"]*/
/*eslint-env es6*/

let foo = () => {
    return 0;
};

let foo = (retv, name) => {
    retv[name] = true;
    return retv;
};
```

The following patterns are not considered problems:
```
/*eslint arrow-body-style: ["error", "never"]*/
/*eslint-env es6*/

let foo = () => 0;

let foo = () => ({ key: 0 });
```
@btmills btmills 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 Jun 3, 2016
@btmills
Copy link
Member

btmills commented Jun 3, 2016

Thanks for the suggestion, @ajhyndman! "never" seems like an appropriate complement to the current "always" and "as-needed" options. @eslint/eslint-team thoughts?

@ilyavolodin
Copy link
Member

Seems reasonable to me. Although I'm not sure if this is going to be used all that often.

@ajhyndman
Copy link
Contributor Author

If I mention that this rule is espoused by Douglas Crockford, does that strengthen or weaken my case? :)

http://www.jslint.com/help.html#function

@pedrottimark
Copy link
Member

As we consider whether this should be accepted, let’s make sure the option name and explanation of use case makes sense related to #5936 which is accepted but still needs bikeshedding at this time.

@btmills
Copy link
Member

btmills commented Jun 3, 2016

@pedrottimark raises an excellent point. #6216 (the PR for #5936) looks like it will add a boolean requireReturnForObjectLiteral option to the "as-needed" mode. It's a different approach to solve the same ambiguity, @ajhyndman is that sufficient for your use case?

@ajhyndman
Copy link
Contributor Author

ajhyndman commented Jun 3, 2016

@btmills I agree, #5936 identifies the same ambiguity and proposes a good solution for addressing it.

I guess I would opine that enforcing arrow functionsimplicit return and function expressionsblock body is a slightly tidier solution to the problem. It's also interesting that the option configuration falls out so naturally.

If you're asking me, I'd prefer to use this version of the rule.

@nzakas
Copy link
Member

nzakas commented Jun 3, 2016

I think we can support "never" along with #5936. I'll champion this.

@nzakas nzakas self-assigned this Jun 3, 2016
@alberto
Copy link
Member

alberto commented Jun 4, 2016

What's proposed here is not equivalent to #5936 and as @pedrottimark mentioned, the description is not accurate of the behaviour this would have.

never would warn any block-style arrow function, thus preventing arrow functions containing more than one statement.

@ajhyndman
Copy link
Contributor Author

@alberto I agree, one feature does not make the other redundant.

That's correct, never would warn of any block-style arrow function. I want to prevent arrow functions from containing more than one statement.

Is my description not clear enough? Let me know if I can adjust anything.

@kaicataldo
Copy link
Member

kaicataldo commented Jun 4, 2016

@ajhyndman I think they're referring to the fact that your original post says the reason you want this enhancement is

Arrow functions that return object literals can look very similar to arrow functions with brace bodies. Some syntactic ambiguity can be avoided by disallowing block-style arrow functions in favor of ES5 function expressions.

The other proposal specifically targets the case you cited as the problem case above. But what your proposal really enforces is disallowing arrow functions containing multiple statements (and always forcing the use of implicit returns), which reaches far wider. Though it does have the side effect of solving the problem you cited.

I think this is a reasonable addition, though I don't know how often it'll be used. It disallows the use case of multi-statement arrow functions that are being used to retain their reference to their parent scope's context, which, in my experience, is pretty common.

@ajhyndman
Copy link
Contributor Author

@kaicataldo I think your appraisal is fair. My initial description focuses on what I thought was the most straightforward and clearly defined benefit. You're also right that it won't make sense in a lot of projects which use => purely as a shorthand for binding.

While it's not a critical point. I'd like to take a moment to try to dispel the notion that I'm not aware of what I'm proposing. I think "forced-implicit-return" function syntax is actually a growing trend in modern programming languages.

Coffeescript's arrow functions do technically allow multi-statement function bodies, but they heavily favour single-expression implicit returns.
http://coffeescript.org/#literals

Elm strictly enforces side-effect-less functions that always return a value. The clarity and compiler optimizations this style of programming allows are a major feature of the language.
http://debug.elm-lang.org/

In Lisp dialects (including Closurescript) everything is interpreted as an expression, not just function bodies.
https://github.com/shaunlebron/ClojureScript-Syntax-in-15-minutes

In Javascript, we have a lot of imperative APIs to deal with, and side-effect-less code is impossible to completely avoid. I simply find that restricting the role of arrow functions to that of a shorthand for single-statement implicit returns improves readability of my code. ES5 function expressions always contain multi-statement block style bodies. The two distinct syntaxes now carry two clearly distinct meanings.

One common mistake that I run into when writing this way is accidentally omitting the parens around a literal object return! And that's the story of why I'm here bothering you fine individuals.

We agree that this option may not be desirable in all projects, some of which rely on arrow functions for scope binding. I think making this option available would be a win for codebases that are interested in adhering to a more state-less, side-effect-less functional pattern.

@kaicataldo
Copy link
Member

Oh, I don't think anyone was thinking you weren't aware of the implications of your proposal - we just want to make sure we evaluate all proposals fully and are all clear on what they entail while we discuss whether they should be accepted or not :)

@nzakas
Copy link
Member

nzakas commented Jun 5, 2016

We've gotten a bit off track here. We have a champion and two 👍s from the dev team (@ilyavolodin and @kaicataldo). We need one more to accept this. Anyone want to give it?

@btmills
Copy link
Member

btmills commented Jun 5, 2016

👍

@nzakas nzakas 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 Jun 6, 2016
@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