Tslint: member-ordering and semicolon false positive for function fields

[buu700]

Bug Report

• __TSLint version__: 5.8.0
• __TypeScript version__: 2.5.3
• __Running TSLint via__: CLI

TypeScript code being linted

export class Balls {
    public readonly foo = () => {
        this.bar();
    };

    public readonly x = 'x';

    public bar () {
        doStuff();
    }
}

with tslint.json configuration:

{
    "rules": {
        "member-ordering": [true, {"alphabetize": true, "order": [
            "private-static-field",
            "protected-static-field",
            "public-static-field",
            "private-static-method",
            "protected-static-method",
            "public-static-method",
            "private-instance-field",
            "protected-instance-field",
            "public-instance-field",
            "private-instance-method",
            "protected-instance-method",
            "public-instance-method",
            "constructor"
        ]}],
        "semicolon": [true, "always"]
    }
}

Actual behavior

Declaration of public instance field not allowed after declaration of public instance method. Instead, this should come at the beginning of the class/interface.

'bar' should come alphabetically before 'foo'

Unnecessary semicolon

Expected behavior

No errors, as foo is a field with a function value, not a method.

[ajafff]

This has nothing to do with the semicolon. If you have a problem with that, you should use "ignore-bound-class-methods" in the semicolon rule.

member-ordering treats properties initialized with an arrow function (=bound class method) like any other method. That's why you get that error.

[buu700]

Well, it isn't a class method, since there's a semantic difference between the two. Testing the following:

(<any> window).scope = 'global';

class Balls {
    public readonly scope = 'class';
    public readonly foo = () => { console.log(this.scope); };
    public bar () { console.log(this.scope); }
}

const {foo, bar} = new Balls();
foo();
bar();

It first prints "class" then "global".

I also reproduced the same issue when importing a function from somewhere else and making it an instance field, e.g.

import {doStuff} from './do-stuff';

export class Balls {
    public readonly doStuff: typeof doStuff = doStuff;
}

[buu700]

By the way, this bug is more of a hassle after TypeScript 2.7.

With 2.6, this pattern both compiles and lints against member-ordering with no issues:

export class Balls {
    private resolveThing: (thing: Thing) => void;

    public thing = new Promise<Thing>(resolve => {
        this.resolveThing = resolve;
    });

    ...
}

Thanks to the new strictPropertyInitialization, that will no longer compile as-is. The simplest fix is to rewrite the second line as private resolveThing: () => void = () => {};.

However, this triggers the issue where resolveThing is misidentified as a method rather than a field of type function, causing a lint failure.

---

Edit: That specific fix isn't great because it makes the code more fragile and causes it to break when reversing the order of the two members, but the better fix I landed on has the same issue with this lint rule, so the point remains the same.

[Zarepheth]

I'm having the same problem with an Angular 5.x component. I have an optional @Input field that should accept a function. For consistency, I place fields decorated with @Input, @Output, and such at the top of my component. However, I want this Function typed field to default to a specific function. To accomplish that, I use an arrow expression.

Even though it is semantically a Field, TSLint treats it as a Function member; thus spawning a series of errors because private instance fields are declared after it (where they belong).

A hasty example (not actual code)

@Component (...)
export class MyComponent implements ... {

    @Input()
    public MyFirstField: number | null = null;

    @Input()
    public MyFunctionField: (param1: Params | string ) => Observable<MyType>
        = (param1: Params | string) => this._myService.ServiceCall(param1);

    public MyPublicField: string;
    private myPrivateField: string;

    constructor(private readonly _myService: MyService) { }

    public MyMethod1() {
        ...
    }

    private myMethod2() {
        ...
    }

}

My IDE's Intellisense popups recognize MyFunctionField as a field, even though TSLint treats it like a method. So each public field, private field, and the constructor which follow the Function Field generates a TSLint error because they are not in proper sequence - however, they actually ARE in proper sequence.

[Zarepheth]

As a workaround, I declare but Do Not Initialize the Function Field. Then I test for and initialize the Function Field just before I use it.

    @Input()
    public MyFunctionField: (param1: Params | string ) => Observable<MyType>; // no default initialization

    ...

    public MyMethod1() {
        if(!this.MyFunctionField) {
            this.MyFunctionField = (param1: Params | string) => this._myService.ServiceCall(param1);
        }
    }

This a bit of a hassle and will be worse the more places one needs to test for and initialize the Function Field.

[JoshuaKGoldberg]

Seems reasonable to take in an option in the member-ordering rule for this requested behavior.

[mgol]

Issue #4156, which has just been closed as a duplicate of this one, shows a case where current member-ordering requirements are impossible to achieve. Therefore, for me, it's not a question of code style tastes which may vary but a real bug in the rule definition.

Therefore, I'm not convinced just adding an option to the rule is a complete solution. No requested ordering should be impossible to achieve.

[buu700]

I don't really like it, but in those scenarios you can also initialize fields in the constructor, e.g.

class C {
    readonly debouncedReturnValue: T;

    readonly returnValue = () => this.value;

    value = 42;

    constructor () {
        this.debouncedReturnValue = debounce(this.returnValue, 1000);
    }
}

I think an option makes sense here, since it isn't actually impossible. You'd then, as far as I can tell, have to choose from three options: constructor initialization, lint exception, and a lint rule option to automatically make those exceptions when in this situation.

[mgol]

Good point about the constructor workaround. It's not that nice with TypeScript, though as it splits the type definition from initial assignment which allows for some errors (if you use the value before its assignment) and doesn't allow to rely on type inference.

[JoshuaKGoldberg]

💀 _It's time!_ 💀

TSLint is deprecated and no longer accepting pull requests other than security fixes. See #4534. ☠️
We recommend you instead use typescript-eslint to lint your TypeScript code with ESLint. ✅

👋 It was a pleasure open sourcing with you!

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 _(#4534)_ and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋