Prefer arrow functions

[jhnns]

Over the past years, my personal preference on functions vs. arrow functions has shifted towards arrow functions. I used to be against arrow functions as a general replacement as they behave different than regular function. Typical arguments against arrow functions in general were:

1. They don't provide arguments
2. this is derived from the upper scope
3. They are not callable via new
4. They don't have a prototype
5. They are not hoisted, but are only available after evaluation
6. They appeared as anonymous function in stack traces (.name was undefined)

But now I've changed my mind:

1. With rest parameters, arrow functions can have arguments. Rest parameters are better because they are actual arrays. And in general, I think that all these arguments and rest parameters tricks can be replaced with regular array arguments which are more explicit and expressive anyway.

2. I never use this in regular functions. Functions that use this should be part of a class where you can use method shorthands without the function keyword. A function that uses this and that is not part of a class should rather accept an explicit argument than an implicit this (we already enforce this with no-invalid-this).

3. This is a good feature. If a function should be callable with new, it should be a class

4. Same as above

5. This is never a real issue. If it is, it can be replaced with simpler and more explicit code. Not true. This may become an issue when evaluating modules that are part of a cyclic dependency graph.

6. This is not true anymore. JavaScript engines derive the function name from the variable name:

---

Benefits of preferring arrow functions:

• Not accidentally callable with new (already mentioned)
• They discourage strange usage of this (already mentioned)
• Unified function style (= no special rules that arrow functions are only appropriate in certain scenarios)
• Shorter one line functions (no return) which are very common in functional programming patterns like Redux' action creators (see also #22)

The two last arguments are the most important ones for me that motivated me to change the rule. What do you think about it?

[bytetyde]

I'm not sure I understand your first argument against arrow functions. What do you mean with they don't provide arguments?

Generally I like arrow functions, for me the shortness and the auto-binded this context are really good benefits which make them way more convenient to use in the majority of use cases than normal functions.

[leomelzer]

@Bluebyt I think @jhnns meant the Array-like arguments-object:

> const blub = () => console.log(arguments)
undefined
> blub("foo", "bar")
ReferenceError: arguments is not defined
    at blub (repl:1:32)
> function blub2() { console.log(arguments) }
undefined
> blub2("foo", "bar")
{ '0': 'foo', '1': 'bar' }

[acidicX]

I think it can sometimes lead to a visual mess, e.g. if an arrow function returns an arrow function.

This:

export const createGetRecipeTileImage = (images, imagesHighDpi) => (
  recipeImage,
  h,
  w
) => { ... }

is much less readable than this:

// note that it is immediately clear that this is a factory
// because you can read "return function" inside the code
export function createGetRecipeTileImage(images, imagesHighDpi) {
  return function (recipeImage, h, w) { ... }
}

However, it only seems to be bothering me at SmartFood :)

[moalo]

My thoughts:

• In general I prefer the function keyword for functions that really do something. It feels more self-explanatory and semantically correct to me.
• I think it's ok to use const with arrow functions for things that don't contain logic and just return stuff. const selectFilters = state => state.recipes.filters
• Sometimes this doesn't work out, as I need to use the arrow function because I need the auto-binded this.
• I can totally relate to @acidicX's point. But maybe this has to do with gesunder Menschenverstand as well.

[bytetyde]

@leomelzer got it. But do you often have use cases for this?

The nested arrow function confusion comes from using anonymous functions, but this can be reduced if the functions are saved with meaningful names, can't it?

export const createGetRecipeTileImage = (images, imagesHighDpi) => {
  const someNiceName = (recipeImage, h, w) => { ... }
  return someNiceName

This is clearer in both cases and a preferable way, isn't it?

[jhnns]

@acidicX

I think it can sometimes lead to a visual mess, e.g. if an arrow function returns an arrow function.

I agree with you that your example is hard to read. But I don't think that it's a problem of the arrow function itself, more a problem how prettier + ESLint decide how this should be formatted. For instance, you could format it like that:

export const createGetRecipeTileImage =
    (images, imagesHighDpi) =>
    (recipeImage, h, w) => {
        ...
    }

which I think is even more readable than the function example. Maybe we can tweak ESLint a little bit so that it refactors it to that format.

@el-moalo-loco

In general I prefer the function keyword for functions that really do something. It feels more self-explanatory and semantically correct to me.

I know what you mean, but the distinction is hard to define and probably impossible to describe as a linting rule. It would probably be: If the function returns something, it's probably a shorter arrow function, if the function has side-effects, it's probably longer and a regular function.

But on the other hand, it's also strange to see both function styles scattered across the code base. I'd prefer a unified function style. My assumption is also that you become used to scanning for the => instead of the function.

[jhnns]

I've added them as custom style (not mandatory). We can try it out in a real-world project.

[jhnns]

With the popularity of TypeScript this is becoming more important. I would at least change our function style from declaration to expression:

// current
function myFunction() {}
// new
const myFunction = function () {};
// or more concise
const myFunction = () => {};

The reason is TypeScript's contextual typing:

// children and the return type is correctly inferred
const MyComponent: React.SFC = ({children}) => {
    /* ... */
};

or

// req, res, next can be inferred
const expressMiddleware: Middleware = (req, res, next) => {

};

I would definitely like to change that rule for our TypeScript linting rules. But since it is awkward to use different styles in TS and non-TS projects, I would also like to change our base rules.

Since this rule is not autofixable, we can leave the old way as accepted "style". You would just have to extend peerigon/styles/prefer-func-declaration.

What do you think @leomelzer @bytetyde @acidicX @el-moalo-loco @meaku @mksony?

[mksony]

Agree, I think with the rise of typescript function expressions are a good choice, also +1 from my side for the consistency with the js rules.

The only thing which I would consider a downside is that examples for generics or type guards in the typescript docs actually use function declarations https://www.typescriptlang.org/docs/handbook/advanced-types.html. So if someone is new to typescript the entrance hurdle might be a little higher if we enforce this style as an error.

[jhnns]

I recently discovered another downside of function expressions in combination with module exports. There are certain situations where you might run into a runtime error during application startup. The main ingredients are:

• A circular dependency between two modules (A & B)
• An exported function expression from module A
• Module B that imports that function expressions and uses it during module evaluation

---

An example:

// a.js
import "./b.js"; // depend on b.js for whatever reason

export const onClick = () => {};

// b.js
import {onClick} from "./a.js"; // this creates the circular dependency

// Now let's execute some code during module evaluation
document.body.addEventListener("click", onClick);

With a function expression, onClick will be undefined when the event listener should be registered because onClick is in the temporal dead zone (TDZ) at that point. With a function declaration, onClick is a function because the function gets hoisted.

We ran into a similar problem at one of our projects recently. In our example, we had a circular dependency and we created Redux selectors using createSelector from the reselect package:

export const getEditingMode = createSelector(
    getState,
    documentStore.isLatestVersion,
    documentStore.isDraftVersion,
    documentStore.isLocked,
    // ...
);

In certain situations, createSelector would complain that documentStore.isLatestVersion is not a function.

---

Personally, I think these problems are solvable. Circular dependencies can usually be avoided by refactoring modules. E.g. you can easily put the problematic code into a dedicated module which both modules can depend on. In fact, this is usually the better choice anyway.

However, I think this is the biggest downside of using function expressions I encountered so far.

[jhnns]

Related discussion at the TypeScript repository: https://github.com/Microsoft/TypeScript/issues/16918

Would be nice to get contextual typing with function declarations.

[hpohlmeyer]

I just ran into a problem with the prefer-arrow-functions rule in typescript with JSX.

If we use it with generics, we have to extend the generic or use multiple type parameters to avoid conflicts with JSX.

// has conflicts
const myFn = <T>(value: T): T => value;

// works
const myFn = <T extends any>(value: T): T => value;

This is especially bad if we really want to allow any value, since we do not allow any.

const isDefined = <T extends any>(value: T): NonNullable<T> => value != null;

More info: https://github.com/Microsoft/TypeScript/issues/4922

[jhnns]

Oh shit... that sucks :(. Strange that I haven't encountered this limitation so far. I still think that the benefits of function expressions outweigh the drawbacks, but that's the biggest blocker so far.

What solution would you recommend @hpohlmeyer?

[jhnns]

The best fix for this is to use a dangling comma in the type parameter list:

const myFn = <T,>(value: T): T => value;

Not beautiful, but at least it doesn't require you to use any.

I still think that function expressions are better (at least in combination with TypeScript because of contextual typing). What do you think?

[hpohlmeyer]

Oh that is a nice workaround! It does not look too bad imo.
Function expressions are still fine with me and this solves my issue with them. :)