Migrate to eslint

[kjeremy]

TSLint is deprecated and we should move to ESLint like vscode did in the 1.42 release.

See: https://code.visualstudio.com/api/advanced-topics/tslint-eslint-migration

[meldron]

I already tested the current code base against eslint:recommended and plugin:@typescript-eslint/recommended and there are around 100 problems, which are all relatively easy to fix.

Especially the errors according tot he rule Missing return type on function (@typescript-eslint/explicit-function-return-type) should be fixed because they lead to errors over time.

If there is an interest in that switch, I would volunteer to create a pull request.

[lnicola]

Please do. I don't think anyone is working on this.

[meldron]

Okay! I will start working on it today.

[Veetaha]

@meldron , may I ask us please not to introduce new lint rules in the first PR. I am, pesonally, not sure that we need the whole of explicit-function-return-type rule... If you'd like to add that, let's discuss this in other issue or PR.

[meldron]

@Veetaha , the rule explicit-function-return-type is part of plugin:@typescript-eslint/recommended, so I am not sure how to proceed, because if we would not use the recommended rule sets (which i would recommend) we would have to try to create a rule set combination which is like the one tslint was using (which imo is not a sound approach).

May be one approach would be to list all rules the current code bose is violating and then decide which of these rules should be ignored and fix the rest?

[lnicola]

Maybe you could file a PR fixing those before the eslint migration, but I don't feel strongly about it. IMHO, more types is better ( :-) ), and I'm not sure it's worth fighting with or over the defaults.

But please don't mix fixes for multiple rules in a single commit.

[Veetaha]

@meldron, I mean, yes, the idea to use recommended lint rules sounds reasonable. I just want to prevent mixing two tasks into one (migrating to eslint and migrating to stricter lint settings). I like the idea of listing the rules which we are violating and explicitly disabling them, so we can discuss whether we do need to fix them afterwards.

[kjeremy]

I am fine with all the changes in one PR. If splitting them out is a concern maybe that can be done in separate commits but I don't think it's a big deal if not.

…

On Tue, Feb 11, 2020, 8:40 AM Veetaha ***@***.***> wrote: @meldron <https://github.com/meldron>, I mean, yes, the idea to use recommended lint rules sounds reasonable. I just want to prevent mixing two tasks into one (migrating to eslint and migrating to stricter lint settings). I like the idea of listing the rules which we are violating and explicitly disabling them, so we can discuss whether we do need to fix them afterwards. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3041>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBACRC4W7LTVIY2GH3KZNDRCKTFXANCNFSM4KREN6DA> .

[meldron]

Okay, so I will create a PR and add eslint first with recommended rules and post all offending rules and then will do a commit per rule to fix the violations. If it is decided to not follow one of those rules the changes can be easily reverted.

[kjeremy]

Fixed in #3209

[matklad]

We've enabled minimal eslint.

I've also tried the recommended set, but it fired warnings which I find counter productive:

• "X was used before it was defined": writing code top-down is important for readability, and all code is in functions, we use proper module and don't execute code in global scope.
• "Forbidden non-null assertion": having this as a lint is just backwards IMO. Non-null assetion is exactly the tool to specify "hey, I want to subvert type-system here", it's already a way to explicitly opt-in into the behavior.

So I'll be strongly against enabling this set of rules. I am also strongly against spending time on finding our own reasonable profile. We should stick to the exremely minimal profile, which basically just normalizes the use of semicolon (I wish just tsfmt can do that), and maybe a rule about floating promises.

[Veetaha]

I'd also add this rule. For example such an issue appeared in vscode codebase

[meldron]

Sorry I have been too slow for you guys.

Why not just disable those two rules and use the rest?

For floating promises there is @typescript-eslint/no-floating-promises

//...
rules: {
"@typescript-eslint/no-floating-promises": "error",
"@typescript-eslint/no-use-before-define": "off",
"@typescript-eslint/no-non-null-assertion": "off",
//...
}