Consider adding more ESLint rules

tdulcet commented 1 year ago

Looking at the over 260 ESLint rules, there have been a lot of potenchally useful ones implemented since this .eslintrc file was created. It would probably be good to add some them to this repository and your other add-ons.

Here are the rules I would specifically suggest adding (listed in the same order as the documentation):

        // Problems
        "array-callback-return": "error",
        "no-await-in-loop": "error",
        "no-constant-binary-expression": "error",
        "no-promise-executor-return": "error",
        "no-template-curly-in-string": "error",
        "no-unmodified-loop-condition": "error",
        "no-unreachable-loop": "error",

        // Suggestions
        "default-param-last": "warn",
        "func-names": "warn",
        "init-declarations": "warn",
        "no-caller": "warn",
        "no-else-return": "warn",
        "no-extend-native": "warn",
        "no-extra-bind": "warn",
        "no-extra-label": "warn",
        "no-floating-decimal": "warn",
        "no-implicit-coercion": "warn",
        "no-invalid-this": "warn",
        "no-lonely-if": "warn",
        "no-new": "warn",
        "no-new-func": "error",
        "no-octal-escape": "warn",
        "no-return-assign": "warn",
        "no-return-await": "warn",
        "no-undef-init": "warn",
        "no-undefined": "warn",
        "no-unneeded-ternary": "warn",
        "no-useless-computed-key": "warn",
        "no-useless-concat": "warn",
        "no-useless-rename": "warn",
        "no-useless-return": "warn",
        "operator-assignment": "warn",
        "prefer-exponentiation-operator": "warn",
        "prefer-named-capture-group": "warn",
        "prefer-object-has-own": "warn",
        "prefer-object-spread": "warn",
        "prefer-regex-literals": "warn",
        "quote-props": ["error", "as-needed"],
        "require-unicode-regexp": "warn",
        "strict": "warn",

        // Layout & Formatting
        "block-spacing": "warn",
        "comma-dangle": "warn",
        "comma-spacing": "warn",
        "comma-style": "warn",
        "computed-property-spacing": "warn",
        "func-call-spacing": "warn",
        "key-spacing": "warn",
        "keyword-spacing": "warn",
        "no-extra-parens": "warn",
        "no-trailing-spaces": "warn",
        "no-whitespace-before-property": "warn",
        "rest-spread-spacing": "warn",
        "space-before-blocks": "warn",
        "space-in-parens": "warn",
        "space-infix-ops": "warn",
        "space-unary-ops": "warn",
        "switch-colon-spacing": "warn"

Many of these would have automatically caught things you have mentioned in your reviews of my various PRs. Using these additional rules to check Unicodify and the Awesome Emoji Picker found a few mostly minor issues.

rugk commented 1 year ago

Sure, feel free to raise a PR. (Or if you wanna, also for the extensions where you find them useful.)

As long as they are not already included in the default set feel free to add them.

tdulcet commented 1 year ago

As long as they are not already included in the default set feel free to add them.

None of the ones in my suggested list above are in their recommended set. When creating that list I tried to configure the rules to follow your existing coding style, but there were a few cases that were not clear, so I went with my personal preference:

"comma-dangle" - This removes dangling/trailing commas, but it could instead require them.
"quote-props" - This removes unnecessary quoting on object literal property names, but it could instead require quoting.
"no-extra-parens" - This removes unnecessary extra parentheses around expressions, but by default it does conflict with your existing "no-confusing-arrow" rule.
"no-continue" - This disallows continue statements to make code more readable/maintainable (see the rule for details).

It also might be good to use a newer more modern JS/ECMA version (such as "latest"): https://github.com/TinyWebEx/AddonTemplate/blob/2068d6231799792cdfa789df8ef8da59e4912744/.eslintrc#L4 It looks like the .eslintrc file should also now be called .eslintrc.json.

rugk commented 1 year ago

This disallows continue statements to make code more readable/maintainable (see the rule for details).

I'm not convinced of that one, it also says “if used incorrectly” that can be a problem. Also it's rather againts the "early return" pattern (that uses return, but you vcan use continue for the same reason, i.e. to prevent more and more nested blocks). There, continue can be useful IMHO.

The other rules are fine.

This removes unnecessary extra parentheses around expressions, but by default it does conflict with your existing "no-confusing-arrow" rule.

Hmm no preference here, Either remove my rule to remove the parens or force them.

As for the modernization/renaming: Sure, go for it. I'd just rather not use "latest", but hardcode the latest version (2020 or so?) in there, as we usually develop against a minimum browser version and the extension needs to stay compatible here.

tdulcet commented 1 year ago

I'm not convinced of that one, it also says “if used incorrectly” that can be a problem.

OK, I removed it from my suggested list above.

I'd just rather not use "latest", but hardcode the latest version (2020 or so?) in there, as we usually develop against a minimum browser version and the extension needs to stay compatible here.

Per the documentation, 2022 is the latest version, but I would probably suggest using 2021 to maintain compatibility with slightly older ESLint versions and Firefox/Thunderbird 91 ESR. Maybe a good policy would be to increment it every year sometime after the new year.

rugk commented 1 year ago

Sure 2021 is fine.

Maybe a good policy would be to increment it every year sometime after the new year.

Well… I'd only increment it if we need a new feature. Otherwise why do so?

Also, what is very clear: The version given there must be supported by the oldest browser mentioned in our manifest.json. Otherwise, we cannot use it as it would break old browsers then. We can of course bump that browser version, so 2021 is supported at least.

rugk commented 1 year ago

https://caniuse.com/sr_es12 is BTW not very helpful :astonished:

tdulcet commented 1 year ago

Well… I'd only increment it if we need a new feature. Otherwise why do so?

Yes, for existing add-ons, but this repository is just a template, so presumably new add-ons would not need to worry about supporting old browsers as much. Note that a few of my suggested rules do require 2021.

Also, what is very clear: The version given there must be supported by the oldest browser mentioned in our manifest.json.

There does not seem to be much coloration between JS/ECMA and browser versions, as browsers frequently implement the new features months or years before they are officially standardized, but sometimes not until after.

https://caniuse.com/sr_es12 is BTW not very helpful 😲

See here (scroll down): https://kangax.github.io/compat-table/es2016plus/

TinyWebEx / AddonTemplate

Consider adding more ESLint rules #6