Using putout linter on xterm codebase

[coderaiser]

Just read about switching eslint rules to warnings, and want to suggest usage results of putout linter that fixes everything it can find.

Here is list of things in code of xterm.js that can be improved:

• convert-for-each-to-for-of (enabled)
• convert-for-to-for-of
• remove-console
• simplify-ternary (enabled)
• remove-empty
• apply-shorthand-properties (enabled)
• extract-sequence-expressions (enabled)
• convert-math-pow
• convert-template-to-strings

Here is used config.putout.json:

{
    "rules": {
        "remove-console": "off",
        "remove-unused-variables": "off",
        "remove-useless-variables": "off",
        "apply-destructuring": "off",
        "merge-if-statements": "off",
        "convert-apply-to-spread": "off",
        "convert-math-pow": "off",
        "convert-for-to-for-of": "off",
        "convert-template-to-string": "off",
        "remove-empty": "off"
    }
}

I'm willing to run putout again disabling rules you choose If it's appropriate , I think this can make codebase of xterm.js a little bit better :).

About an error:

1) CoreMouseService
       triggerMouseEvent
         eventCodes with modifiers (DEFAULT encoding):

      AssertionError: expected [ Array(8) ] to deeply equal [ Array(4) ]
      + expected - actual

       [
      -  "\u001b[M !!"
      -  "\u001b[M!!!"
      -  "\u001b[M\"!!"
      -  "\u001b[Ma!!"
         "\u001b[M#!!"
         "\u001b[M#!!"
         "\u001b[M#!!"
         "\u001b[M`!!"
      
      at Context.<anonymous> (out/common/services/CoreMouseService.test.js:166:27)

I have the same error on master. Have no idea what it is :).

[jerch]

@coderaiser Did you actually look at the replacements? There are several issues with it, see my comments below.

[coderaiser]

@jerch thank you for review :), I did look at replacement but want to receive feedback before proceeding. I’ll fix all comments when get to a computer.

[coderaiser]

@jerch just force pushed updates, we have only 4 rules not disabled, from found list :):

• convert-for-each-to-for-of
• simplify-ternary
• apply-shorthand-properties
• extract-sequence-expressions

extract-sequence-expressions find redundant code in src/common/input/Keyboard.test.ts

Also remove-unused-variables can help to find all cases similar to one find in src/browser/Terminal.ts. I can proceed with this in next PR if there is a need on it :).

[jerch]

@coderaiser
Did a quick microbenchmark comparing for-of with classical loop types known to be fast. Summary: for-of takes ~2 times longer on recent chrome, ~10 times longer on recent Firefox. Perfwise for-of is still smelly and does not look good compared to index loops.

I think for time critical parts we should resort to good old index-for-loops. For non critical paths imho for-of is good to go with it. forEach is way slower than all of them above, and has weird restrictions on the used type. for-of is clearly the better choice here.

@Tyriar Do you see any issues with for-of, like ECMA version supported? Imho it got official with ES6, I am not sure if TS will transpile it for lower targets correctly.

[Tyriar]

Pretty much agree with all your thoughts @jerch. Also I don't think we would want to include an additional linter, I'm trying to slim down out dev deps where possible.

---

The forEach change seemed negative to me at first, but having it not be inside a callback would help TS understand what's going on, specifically in this case which is just annoying to deal with:

I think for time critical parts we should resort to good old index-for-loops. For non critical paths imho for-of is good to go with it.

I have similar thoughts, I wouldn't even bother trying to convert for loops as we typically only use them not for perf critical parts and when we're working with the index.

Do you see any issues with for-of, like ECMA version supported? Imho it got official with ES6, I am not sure if TS will transpile it for lower targets correctly.

It transpiles down to this for es5 targets:

var a = [1, 2, 3];
for (var _i = 0, a_1 = a; _i < a_1.length; _i++) {
    var b = a_1[_i];
    console.log(b);
}

This ends up being ~40% slower in Chromium for me which isn't too bad.

---

remove-empty is the only other rule that I think we may want to run. If that only catches the while (reports.pop()) { }, which would definitely be better written as reports = [] as suggested anyway.

[coderaiser]

@jerch

Did a quick microbenchmark comparing for-of with classical loop types known to be fast. Summary: for-of takes ~2 times longer on recent chrome, ~10 times longer on recent Firefox. Perfwise for-of is still smelly and does not look good compared to index loops.

I think for time critical parts we should resort to good old index-for-loops.

Absolutely agree with you :). The rule convert-for-to-for-of already disabled.

I think for time critical parts we should resort to good old index-for-loops. For non critical paths imho for-of is good to go with it. forEach is way slower than all of them above, and has weird restrictions on the used type. for-of is clearly the better choice here.

Yep, for-of definitely better then forEach :).

remove-empty is the only other rule that I think we may want to run. If that only catches the while (reports.pop()) { }, which would definitely be better written as reports = [] as suggested anyway.

Yes, I'll add it, it is have sense :).

Also I don't think we would want to include an additional linter, I'm trying to slim down out dev deps where possible.

@Tyriar I don't suggest to add an additional linter :). I suggest to add results of it work :).

[jerch]

LGTM 👍