MetaMask / eslint-config

Shareable MetaMask ESLint config
MIT License
6 stars 16 forks source link

Enforce use of `function` for declared functions #324

Open mcmire opened 11 months ago

mcmire commented 11 months ago

There are two ways to declare functions:

// 1
function foo(x: number): string {
  // ...
}

// 2
const foo = (x: number): string => {
  // ...
}

I propose that we add a rule which enforces that declared functions use the first style rather the second. There are a couple of issues with using an arrow function that I think function solves:

  1. It's not immediately obvious that the thing being set is a function. function pops out more visually.
  2. Adding type information for arrow functions can be confusing syntactically. (I've seen less senior developers struggle with doing this.) => is a bit overloaded, as it's used to indicate the return value for a function type in TypeScript. When using function, however, you don't have to deal with the ambiguity of => nor do you have extra characters in the way. : always separates the arguments from the return value, and the return value always goes before the {.

There are also some benefits for using function:

MajorLift commented 11 months ago

Edge case for consideration:

The const F: (arg: T) => U syntax for typing functions is only supported by const function declarations, and it can be used for conveniently defining and typing higher-order constructs e.g.

const printCoordinates: (x: number) => (y: number) => `(${string}, ${string})` 
  = (x) => (y) => `(${x}, ${y})`;

console.log(printCoordinates(1)(-1));
// "(1, -1)"

If we stick to the function keyword for this, the type annotations become a lot more spread apart and redundant.

function printCoordinates(x: number): (y: number) => `(${string}, ${string})` {
    return function printCoordinatesForGivenX(y: number): `(${string}, ${string})` {
        return `(${x}, ${y})`
    }
}

But for most cases I agree with preferring the function keyword for the same reasons given in the post.

brad-decker commented 11 months ago

I'm assuming the rule won't be applied for things like props, or functions passed in objects? For example if a function has an argument that is an object, and one of those keys can be a function should we be enforcing declaration of the function outside the object?

callSiteExample({
  prop1: 'foo',
  prop2: () => console.log('bar'),
})

versus

function prop2() {
  console.log('bar');
}

callSiteExample({ prop1: 'foo', prop2 });

There's definitely some cleanliness to the second example, just curious what your thoughts are here.

mcmire commented 11 months ago

@MajorLift Ah yes, thanks for mentioning that edge case. It would make sense that we'd use const in that case.

mcmire commented 11 months ago

@brad-decker Good point! Yes, my thinking was that this rule would only apply to function declarations and not function expressions. I guess a function defined via const is a function expression, though. So perhaps this rule would be difficult to enforce fully. But to answer your question directly, no, I think we wouldn't need to enforce this rule for functions created on the fly like in your example.