New ESLint config #9748

tylerbutler · 2022-04-05T22:51:14Z

Part of #9501.

Update 2022-04-20

After discussing more with @ChumpChief, I am preparing additional PRs to apply fixes for some of the new rules before merging this and publishing a prerelease. This will ease the integration of the new config.

Update 2022-04-12

The latest PR contains the following changes based on feedback in the review:

Changed to Error

@typescript-eslint/func-call-spacing
@typescript-eslint/keyword-spacing
@typescript-eslint/member-delimiter-style
@typescript-eslint/object-curly-spacing
@typescript-eslint/type-annotation-spacing
new-parens
semi-spacing
valid-typeof

Changed to Warn

@typescript-eslint/no-empty-function
@typescript-eslint/no-unsafe-assignment
@typescript-eslint/no-unsafe-call
@typescript-eslint/no-unsafe-member-access
@typescript-eslint/no-unused-expressions
@typescript-eslint/require-await
@typescript-eslint/restrict-template-expressions
unicorn/consistent-function-scoping
unicorn/expiring-todo-comments
unicorn/prefer-top-level-await

Disabled

@typescript-eslint/no-parameter-properties
@typescript-eslint/sort-type-union-intersection-members

Other changes

@typescript-eslint/explicit-function-return-type -- set allowConciseArrowFunctionExpressionsStartingWithVoid to true
import/no-unresolved -- set caseSensitiveStrict to true

This PR updates our shared lint config by consolidating the three separate configs into a single config, shared-config.js. This config is much stricter than the current minimal config. Every rule that we override is listed alphabetically in the config, along with a brief summary of the intention behind the rule.

A plea

Choosing default lint rules is inherently an opinionated exercise. In the absence of feedback, my opinions regarding lint defaults will become the "law of the land," and that should scare you as much as it does me. 😄 Please let your own opinions be heard!

All that said, after this is merged, I'll follow up with another PR to apply the config across the repo. When that happens, you'll have another opportunity to weigh in on the rules. But it's easier to make changes now.

How to review

To ease this review, I suggest reviewing default.json, which is the diff of the final applied config, taking into account all our plugins, parent configs, etc. That is easier than looking at the config file itself since that file is all new.

Q and A

How did you decide what should be a warning vs. an error?

Practically speaking, the difference between an ESLint error and a warning is that errors fail the build, while warnings don't. The default is that everything is an error, and I generally feel that rules that aren't enforced by build breaks aren't useful. However, there are two situations where I feel a warning is better.

First, if the rule is likely to have a lot of offenders in our code, or if fixing the issues is complex and warrants careful code review, then warning is appropriate because it lets us adopt the new config without temporarily disabling rules and then forgetting to ever turn them off again. Warnings have a tendency to get fixed because eventually someone gets annoyed, 😄 Large-scale lint changes are hard to review because there are often so many changes, so warnings enable us to have more targeted changes that are better reviewed. An example of this kind of rule:

        // Parameter properties can be confusing to those new to TypeScript as they are less explicit than other
        // ways of declaring and initializing class members.
        "@typescript-eslint/no-parameter-properties": "warn",

Second, if the rule is more about formatting or style than code correctness, and I was worried about it being too strict or too controversial, I used warnings. Examples of these kinds of rules:

        // Catches a common coding mistake where "resolve" and "reject" are confused.
        "promise/param-names": "warn",

        // Prefer type-only imports where possible,
        "@typescript-eslint/consistent-type-imports": [
            "warn",
            {
                prefer: "type-imports"
            }
        ],

        // Enforces that members of a type union/intersection are sorted alphabetically.
        "@typescript-eslint/sort-type-union-intersection-members": "warn",

These are all opinionated decisions, and I may have been more arbitrary than the above describes. Let me know where you disagree.

How did you decide what formatting rules to enable?

I prefer to focus linting on finding code issues, either in logic or style. Formatting is a different task, better left to dedicated formatting tools like tsfmt, which is what VS Code uses internally when you format a file. Many lint rules regarding formatting require a lot of connfiguration to work as we want, but our "style guide" is pretty simple -- whatever VS Code produces is the "correct" formatting. Configuring lint rules to do what VS Code "just does" makes me sad, which is why we have a tsfmt task for most projects. It isn't running as part of the build (yet), so some formatting-related lint rules are enabled, but they should be the exception.

How do we apply this config to projects with lots of offenses?

You can disable rules as needed to unblock yourself. You should always prefer line-level disables. If there is a contiguous block of the same offense, then using an disable/enable block comment is acceptable. Project-level overrides (these live in .eslintrc.js) should not be used. They're too easy to forget to clean up. The exception to this is test projects, which might have legitimate reasons to adjust rules at the project level.

common/build/eslint-config-fluid/shared-config.js

ChumpChief

I'm maybe halfway through default.json, submitting some initial feedback. Couple broader feedback items:

This PR is making two main changes (1) structure and (2) actual rule changes. It would be easier to review and understand the changes if this were split into two PRs, first just restructuring but changing no policy, and second being the opinionated policy changes.
I want to be super pedantic about formatting, I've spent too much time giving formatting feedback in PRs when the rules have not been enabled, and I'm sure the recipients of that feedback would have loved for eslint:fix to work :-P. If/when tsfmt eventually becomes part of the build pipeline then maybe we move the responsibility, but in the shorter term we should keep these as build failures so our code doesn't decay.