1

I'm in a situation where there is a code base that has TypeScript and ESlint but:

  • There are a lot of type errors (code compiles despite errors using Babel)
  • There are a lot of lint warnings.
  • We may want to introduce some new lint rules to solve some particular problems we have (for example, cyclic dependencies really were causing a problem, so introducing the import/no-cycle rule to prevent it).

We're now at a point where we are introducing a build pipeline so we can prevent people from committing code with errors - but the problem is the code is already not in a position to pass the CI quality check.

e.g. we can't fail on type errors as it currently is, we'd need to fix all of the type errors first.

What's the best strategy to move toward enforcing code quality, without preventing us from using the code in the meantime?

Here's things I've tried or considered:

  • Big shabang just clean up all of the errors now.

    • This is a lot of work. Especially with sorting out type errors you might end up refactoring things, and then that breaks something.
    • Also, it's not like the 'big shabang' approach is something you 'just need to do once'. For example, adding that no-cycles rule would require another big shabang clean up of fixing the imports.
  • Use a tool like husky and lint-staged to enforce code quality on just the files you touched.

    • The problem I ran into with this is having developers just push with --no-verify and then later when people are pulling master into their branch, they get all of the errors.
    • Admittedly that was a problem of enforcing code style on the dev's machine only - presumably there's a tool that do the 'enforce code quality on only the files you touched' on CI servers.
  • Maintain an ignore list - ie. put the rules in, but keep a list of 'these are the files that are currently broken' and slowly reduce it.

  • This article here recommends coming up with your list of ideal lint rules, but turning them off until the codebase is ready.

    • Essentially that's a series of the 'big shabang' solution. I'm not sure how this would work for type errors.

Are there well tested strategies for solving this problem?

2

Essentially what you need is a system to verify that your code is at least, as bad as it was not incrementing the rule violations with new modifications added. There are ways of enforcing this:

  1. Checking the number of violations and verifying these do not increase at merges.
  2. Doing a check during the code review phase.
  3. Doing small shabangs. All rules off, you activate one and fix all violations of that one, you have already one that will make the build fail in the future.
  4. Combining 1 and 3. You activate 1 rule, check the number of violations, verify that this number decreases (or does no increment) after a merge. When the rule has 0 violations you go to the next one.

Have in mind that you need to have a system to enforce this and a team that wants this to happen.

2

It sounds like you have the right general idea, but to offer some specific suggestions:

  • Automatically fix whatever you can. ESLint has a --fix option and knows how to automatically fix many rule violations across an entire codebase. Use it - it's far more reliable than manually making changes, and it's far faster than spending developer time to worry about (manually or automatically) fixing one file at a time.

    You can automatically fix ESLint rules at a time, in separate commits, to help track changes and mitigate risks. Tools like eslint-nibble can facilitate automatically fixing one rule at a time.

  • Use husky and lint-staged to make issues visible to individual developers and provide encouragement and tooling to address issues. Some good articles here: "Progressive JavaScript Linting", "Using lint-staged, husky, and pre-commit hooks to fail fast and early".

    One disadvantage is that touching a file for any reason can cause Husky + lint-staged to start complaining about every rule violation within that file. Your goal is to move things in a positive direction without interfering with developers' workflows or ongoing development; making developers feel like they have to fix everything whenever they touch a file goes against that. It's okay to use --no-verify in this case to bypass hooks and move forward.

  • Set up your CI to enforce that PRs don't add any additional warnings. Specifics here depend on your CI system; e.g., SonarQube has a quality gates feature. I believe that Jenkins' Warnings Next Generation plugin can distinguish between new and pre-existing warnings. Etc.

  • As a variation on your "maintain an ignore list" approach, there are tools to help automatically maintain ignore lists on a granular basis. (I first encountered this with Ruby's Rubocop's .rubocop_todo.yml. In the ESLint world, there's eslint-generate-todo.) Personally, I'm less certain about this approach; I want my linting / code quality to be as zero-effort as possible, and so adding steps to maintain an ignore file seems like it would be harder to get developers on board and involved. But it's at least worth being aware of.

  • Use Prettier. Set it up to run automatically in your editors / IDEs; it's downright magical to by able to completely ignore formatting and superficial coding style as you're programming and have everything look great the instant you hit Ctrl-S. Set up ESLint to ignore anything that Prettier handles. By removing the need for ESLint to worry about any formatting or superficial coding style, you can let it focus on what it's really good at, and you can cut down on the number of rules you have to worry about in husky + lint-staged / CI.

1

We have similar problems enforcing some rules on our codebase that had a lot of legacy in it.

We use merge requests into our main branch. The way to work for us is to have tooling that would indicate whenever a merge request contained modification on lines or functions that contain a linter warning. The CI pipeline would not pass in that case (we can merge anyways, but this have been proven very rare for the devs not to fix pipeline errors). This way, every merge request would have to contain either new clean code regarding linting or clean some other code when it's modifying legacy.

We're pretty happy about the system as it really enforces incremental change on our base. The only burden was to develop tooling that would enable to compare linted warning lines with modification lines.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.