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

func-style like rule #3

Open
jamestalmage opened this issue Apr 14, 2016 · 11 comments
Open

func-style like rule #3

jamestalmage opened this issue Apr 14, 2016 · 11 comments
Labels

Comments

@jamestalmage
Copy link
Contributor

See: http://eslint.org/docs/rules/func-style

My personal preference is to prefer declaration style, as it allows hoisting (and just looks better to me). The only issue is when you want to reassign:

var func = function () {

}

if (someCondition) {
  func = wrap(func);
}

ESLint's func-style rule does not accommodate the above exception when preferring declarations, so it is impractical to enforce.


Note:

You can get around ELint's rule by doing this:

var func; // don't initialize variable here, do it on the next line

func = function () {

}

But that feels impractical.


Do we feel it's worth enforcing func-style in XO? Should we use the builtin plugin to do so, or make our own that is a bit more permissive.

@jfmengels
Copy link
Contributor

When writing in ES5, I prefer the declaration style, when writing with ES2015, I think I preferred (has been a while now) writing expression style for short functions (const double = (x) => x * 2;), and declaration style for longer ones. With the expression one, you have the benefit that you know that the function won't be re-declared elsewhere if you've assigned it to a const variable (and I don't like hoisting btw).

So I'm rather against enforcing it.

@jamestalmage
Copy link
Contributor Author

Arrow functions would be excluded (that's an option even in ESLints plugin).

@jfmengels
Copy link
Contributor

With arrow function excluded... then maybe yeah.
We'll still have to mix decorated stuff like this anyway:

var fn = _.curry(function (...) { ... });

But yeah, why not.

@jamestalmage
Copy link
Contributor Author

The eslint rule (in declaration mode, with arrow functions excluded), only flags a FunctionExpression if it's immediate parent is a VariableDeclarator, so you only run afoul doing var fn = function () {}.

The ESLint behavior makes sense to me, but it doesn't make an exception for variables that are later reassigned. That is a rare occurrence, but if you are already in the habit of using function declarations, then var fn = function () {} is a telegraphs the potential reassignment to the reader.

Jumping back to arrow functions for a second:

I think they should only be allowed for simple functions with implicit returns

// OK
var square = x => x * x;

// NOT OK
var complexFunc = (x, y) => {
  if (x) {
    return y * 2;
  } else { 
    return y * 3;
  }
}

// I would prefer
function complexFunc(x, y) {
  if (x) {
    return y * 2;
  } else { 
    return y * 3;
  }
}

Exceptions for arrow funcs should be made if they use any of this, arguments, super or new.target.

@jamestalmage
Copy link
Contributor Author

A bit dated, but: https://kangax.github.io/nfe/

@sindresorhus
Copy link
Owner

I think they should only be allowed for simple functions with implicit returns

Why? I prefer arrow functions for everything as they're lexically scoped (no dumb implicit this) and you can use const with them to ensure they're never overridden.

@jamestalmage
Copy link
Contributor Author

Well, if you don't need the lexical binding there's no advantage. Actually there may be a performance hit if arrow functions are transpiled (to accommodate the binding, etc) - though that might not even true - not sure if Babel is smart enough to avoid binding if it isn't needed.

Still, I am not dogmatic about it.

This came up because I noticed we're mixing styles across the AVA codebase, and was wondering if there's a good set of rules we can use to describe our preferred style (whatever that is).

@sindresorhus
Copy link
Owner

Yes, I like to figure out a good default we can enforce for this too. I don't feel super strongly either way, just slightly prefer arrow functions. Also nested function declarations looks messy and having everything as variables looks cleaner and more consistent.

// @vdemedes @novemberborn @SamVerschueren

if you don't need the lexical binding there's no advantage.

Still the advantage of const and no hoisting. I consider hoisting a misfeature. I prefer explicitness.

Actually there may be a performance hit if arrow functions are transpiled

Node.js 4 supports arrow functions though. So I don't see it as a big problem going forward.

@novemberborn
Copy link

I tend to use a mix of syntaxes. E.g. in package-hash, because I'm using export function sync I like declaration style for the other functions in the file.

In other files where there are few declarations I'm more likely to use const with an arrow function, but I'm far from consistent about this.

If I'd have to choose I'd prefer const (and in rare occasions let) with arrow functions. Never var. But not for exports… I'm not sure there's a single consistent answer here.

@sindresorhus
Copy link
Owner

I agree that ES2015 exports should use a normal function.

This:

export function foo() {}

Looks better than this:

export const foo = () => {}

@jacekkopecky
Copy link

Related to this (old-ish) issue, I've recently implemented and submitted related PR to eslint, not realizing it had no chance of being accepted: eslint#16159 .

I too prefer the declaration style, with the exception where reassignment is desired, where fat arrow functions look better to me. The current func-style rule in eslint does not allow the exception, my PR does. Shall I repurpose the PR for plugin-unicorn?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants