only-arrow-functions rule should allow named functions

[glen-84]

Bug Report

• TSLint version: 3.14.0
• TypeScript version: 1.8.10
• Running TSLint via: n/a

TypeScript code being linted

function configureContainer(container: Container) {
    // ...
}

with tslint.json configuration:

"only-arrow-functions": true

Actual behavior

non-arrow functions are forbidden

Expected behavior

Named functions should be allowed.

[JoshuaKGoldberg]

I can take a look at this within a day or two.

[JoshuaKGoldberg]

@glen-84 Does #1452 work for you? I went with adding an option for standalone declarations rather than all named functions because of cases such as:

[1, 2, 3].filter(function whyIsThisNamed(num) { return num % 2 === 0; });
                          ^^^^^^^^^^^^^^

[glen-84]

LGTM =)

[jkillian]

Is there a benefit here to use a named function as opposed to const functionName = () => { //... }?

[glen-84]

Hmm. I'm not sure. 😟

[jkillian]

For example, we'd want the rule to prevent the error case below:

class A {
    constructor(private b) { }

    public getMethod() {
        function foo() { console.log(this.b); }
        const bar = () => { console.log(this.b); }

        // return foo;    incorrect :(
        return bar;    // correct!
    }
}

I'm inclined not to add the extra complexity of an allow-declarations option, but if someone has a strong argument for it, I'm open to changing my mind

[danvk]

@jkillian The very-popular AirBnb style uses a setting like this:
https://github.com/airbnb/javascript#arrow-functions

[glen-84]

@jkillian @danvk is right, and it's also more consistent with the ESLint and JSCS rules, that focus on callbacks.

[JoshuaKGoldberg]

You can get some performance benefits to not having to create a new lambda to preserve scope.

Consider the case of a library that allows execution of callbacks with scope:

class Executor {
    private callback: Function;
    private scope: any;
    private args: any[];

    public setCallback(callback: Function, args?: any[]): void {
        this.callback = callback;
        this.args = args;
    }

    public setCallbackBound(callback: Function, scope?: any, args?: any[]): void {
        this.callback = callback;
        this.scope = scope;
        this.args = args;
    } 

    public execute(): void {
        this.callback.apply(this.scope, this.args);
    }
}

In using setCallback, which would necessitate an arrow lambda to preserve scope, you incur the runtime cost of creating a new function and scope.

executor.setCallback(() => this.doStuff());
// ...
executor.execute();

In using setCallbackBound you just pass pointers around. It's a little more efficient, which can be important in runtime-performance-critical applications like games.

executor.setCallbackBound(this.doStuff, this);
// ...
executor.execute();

Because arrow lambdas capture scope you can't use them for places like this where scope is flexible.

[danvk]

@JoshuaKGoldberg you don't have to turn the rule on if it doesn't make sense for your application! In general, if you're using a library (e.g. jQuery) that sets this for its callbacks, then this rule is not for you.

[JoshuaKGoldberg]

@danvk true! I see libraries like that as a reason why it could be useful to add flexibility to the rule.

[danvk]

Indeed! Naming the callback seems like a reasonable enough escape hatch from this rule.

[jkillian]

Okay, I'm good with this, full-steam ahead on the PR 🚢