Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[eslint-patch] Design Proposal: globally suppress retroactive ESLint errors for legacy codebases (with demo PR) #4303

Closed
kevin-y-ang opened this issue Aug 30, 2023 · 0 comments

Comments

@kevin-y-ang
Copy link
Contributor

kevin-y-ang commented Aug 30, 2023

Implementation

Demo PR located here

Problem Summary

When introducing new ESLint rules to a large, old codebase, the ESLint rule often retroactively reports hundreds to tens of thousands of issues in the existing codebase.
This is problematic for several reasons:

  1. ESLint output is cluttered and difficult to read. Developers may overlook new ESLint issues (untested and likely buggy) because they are overwhelmed with old issues (legacy code, likely tested, stood the test of time in production)
  2. Makes MR pipelines that block ESLint warnings/errors impractical. If your package suddenly has hundreds of new errors, it is impractical to fix them all in a short period of time.

Current solutions to this problem use ESLint disable directives, but these solutions comes with their own issues.

  1. Adding // eslint-disable-next-line rule-name above each retroactive error: This not only makes code ugly and hard to read, but also normalizes abusing // eslint-disable-next-line when writing new code instead of fixing the ESLint errors.
  2. Adding /* eslint-disable rule-name */ at the top of the file: While this solution may work if the entire file is legacy code, it becomes counterproductive if you try to edit the code or add new code, as you may unknowingly introduce new ESLint errors.
  3. Turning off rules for the entire package in the .eslintrc.cjs file: By turning off ESLint rules you are giving up the benefits of ESLint and are likely to write error-prone code.

Proposal

Inspiration

Inspired by .NET code analysis "global suppressions" feature which globally suppresses code analysis annotations, we want to bring this functionality to JavaScript/TypeScript/ESLint. Our proposed name is "Bulk Suppressions". CSS Selectors uses a similar mechanism to select HTML elements with pinpoint precision and granularity.

Typical Use Case For This Tool

  1. Scenario w/ Status Quo: A developer for a legacy codebase wants better code quality, so they introduce new ESLint rules to their repository. Suddenly, hundreds to tens of thousands of ESLint warnings/errors appear. The codebase pipeline blocks all merge requests from all developers with errors. VSCode highlights everything with red and yellow squiggly lines, making code difficult to read and causing developers great distress. If nothing is done to fix it, other developers who work in the codebase will become complacent and condition themselves to ignore all ESLint warnings/errors.
  2. Scenario w/ Bulk Suppressions: Instead, suppose a person introduces new ESLint rules into a legacy codebase, and uses this tool to immediately records all retroactively introduced ESLint warnings/errors in a "bulk suppressions", hiding them from ESLint output. We still get most of the benefits of ESLint. Any new code written will be annotated by ESLint and fixable in bite-sized portions. We can utilize codebase pipelines to block newly written error-prone code without blocking legacy code that has been battle-tested.

Design

Each .eslintrc.js file has a corresponding bulk suppressions file (proposed name: .eslint-bulk-suppressions.json), structured like this:

{
  "suppressions": [
    {
      "file": "MyClass.js"
      "scope": ".class.method.object",
      "target": ".MyClass.myMethod.myObject",
      "rule": "no-invalid-this",
    }
  ]
}

This entry would suppress the ESLint error in the following code:

// MyClass.js
class MyClass {
    myMethod() {
        const myObject = {
            prop: this // This could trigger a "no-invalid-this" rule
        };
        // more code...
    }
    // more methods or properties...
}

Suggested Design Decisions

  • Resilience to Superficial Code Transformations: Scope selectors ensure that the suppression mechanism points to the same code error regardless of superficial code transformations—such as applying formatting rules with Prettier, or adding/deleting new lines at the top of files. These selectors work based on the code's syntax tree rather than exact line and column numbers, so they stay valid as long as the underlying structural logic stays the same.
  • Scoped and Targeted Suppressions: Inspired By .NET code analysis global suppressions, the “scope” and “target” properties use period-separated levels to represent the path to the error location in the codebase. Using a single period (".") to indicate top-level or global errors in both scope and target prevents potential naming conflicts with actual code identifiers or keywords.
  • Localized Suppressions: Each .eslintrc.js file corresponds to its specific suppression file (.eslint-bulk-suppressions.json). This strategy promotes localized and organized handling of suppressions whilst preventing one large, strenuous-to-manage bulk suppression file. It also goes with the flow of ESLint’s configuration being on a per-directory basis.
  • Optional Properties: “justification” or “date” properties
  • Alternative Names to “Bulk Suppressions”: eslint-global-suppressions, eslint-mute, eslint-ignore, eslint-legacy-suppressions, eslint-pardons, eslint-exemptions
  • Alternative Selector Logic: Copy VSCode collapse and sticky scrolling features

Drawbacks

  • Sensitivity to Naming and Structural Changes: Suppressions become invalid if their targeted code structure or names (files, functions, variables) are changed, or if nesting is modified. Similarly, it doesn’t work on code copied to a different function or file.
  • Complexity: The "target" value is human-readable, but the selector algorithm is opinionated and may be difficult to comprehend. For best results, it needs to be machine-generated, which may make troubleshooting more challenging.
  • AST Parsing Required: This tool parses the Abstract Syntax Tree (AST) to apply suppressions. ESLint's existing scope API also relies on AST parsing, but it's designed for use by developers with an intimate understanding of ASTs. Meanwhile, this suppression mechanism needs to be readily accessible to developers of all experience levels.
  • Imperfect Suppression Granularity: Because of JS/TS quirks like anonymous classes and functions, it’s not uncommon for a single suppression entry to suppress more than one ESLint warning/error.
  • Monkey Patching: Similar to modern-module-resolution, our current implementation patches ESLint in a way that wasn’t intended. This approach inevitably results in backwards and forwards compatibility issues with versions of ESLint.

Usage in TikTok Frontend Monorepo

We’ve had lots of success with our implementation of this mechanism at TikTok. The TikTok frontend monorepo consists of almost 1000 packages and around 600 developers. We are currently using our implementation of this tool in more than HALF of all packages and are suppressing over 70,000 ESLint warnings (we don’t suppress errors, @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-function-return-type).

  • Given that we recently introduced a new code style guide to our monorepo, the tool allowed us to automatically label the retroactively introduced ESLint warnings as “legacy” for each package in the form of a .eslint-bulk-suppressions.json file.
  • We use an MR pipeline to block developers from merging code with ESLint errors and this tool allows us to not clog the pipeline with tens of thousands of legacy warnings, and actually use it on new code rather than just turning it off or spamming // eslint-disable-next-line.
  • It gives us easy to consume JSON files that give us useful metrics on our monorepo code quality, and allows comparisons between legacy code and new code.
  • We’ve been using it for several months now and have exactly zero complaints about it, which for an infra team at a big company is definitely a good thing!

Future Improvements

  • VSCode extension: seamless suppression workflow like .NET with Visual Studio by right clicking or hovering, and opt-in white squiggly lines for globally suppressed ESLint warnings
  • Clean up tool: Run a CLI command to clean up .eslint-bulk-suppressions.json values that are no longer applicable
@kevin-y-ang kevin-y-ang changed the title [eslint-patch] Design Proposal: globally suppress retroactive ESLint errors for legacy codebases [eslint-patch] Design Proposal: globally suppress retroactive ESLint errors for legacy codebases (with demo PR) Sep 7, 2023
@iclanton iclanton moved this to Needs triage in Bug Triage Oct 2, 2023
@iclanton iclanton moved this from Needs triage to General Discussions in Bug Triage Oct 2, 2023
@github-project-automation github-project-automation bot moved this from General Discussions to Closed in Bug Triage Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

1 participant