At neeto we are working on 20+ products simultaneously. While we were developing multiple products, we aimed to make all the products look like they were built by a single developer. They should have a consistent coding style and should follow our standards & best practices.
The backend code was fairly clean because of the convention over configuration philosophy of Ruby on Rails framework. But, since React is just a JS library, developers had the complete freedom to write code in all possible ways JavaScript would allow.
In the initial stages, when we were building only 4-5 products, the only enforcement mechanism we had was pull request reviews apart from basic linting using ESLint and code formatting using Prettier. PR reviews quickly turned inefficient as the number of products grew. The code reviewers started missing some of the new changes to the coding standards and continued suggesting the outdated standard. This lead to inconsistent code and behavior between products.
At that time, we thought of building our custom ESLint plugin. Initially, we expected it to be very hard because of the need to deal with language AST to identify the code that didn't follow our standards. However, after some research, we discovered a FOSS tool, astexplorer, which shows a visual representation of JavaScript AST. With that, writing ESLint plugins for non-standard code became easy.
The custom plugin was a huge success during the POC itself. It helped us significantly in improving the quality of the front-end code. At the time of writing this blog, we have 50 custom ESLint rules enforced by the plugin.
The plugin was named eslint-plugin-neeto adhering to ESLint's plugin naming conventions. Later, we namespaced it under @bigbinary, to be consistent with our other frontend packages. Now the plugin is available as @bigbinary/eslint-plugin-neeto.
Enforcing ESLint at neeto
In the overall development flow, ESLint will run on three levels.
-
IDE extension (optional)
Even though BigBinary doesn't have any IDE preference, the majority of developers use Visual Studio Code here. The popular ESLint extension on VS Code runs ESLint while the developer is writing code and gives immediate visual feedback with squiggly lines.
The use of IDE extension is completely optional. We have enforcement mechanisms at other levels to ensure the code quality & standards.
-
Pre-commit hook
There are some ESLint rules like sort-imports that are only meant to keep code consistency. Even though, functionality-wise, there is nothing wrong with keeping the imports in any order, this rule would throw an error if the imports are not correctly sorted.
Secondly, developers will see this error frequently because most IDEs add the new import statement to the bottom of the imports list during auto-import.
But, we believe that developers should not be concerned about errors like these. They should be focusing on the business logic rather than spending time on things that are auto-correctable. At the same time, we need this rule to be auto-fixed at some point to keep consistency.
So, we chose Git's pre-commit hooks for automatically fixing such errors. We used husky to add eslint --fix command to the pre-commit hook. Also, we used lint-staged to run the commands only on the files that are included in the current commit. Thus we automated lint-fixing and formatting of the files that we are going to commit.
If any rule violations aren't auto-fixable, the commit would fail with an error. Developers can get the details of the rule violation from the error message, make corrections themselves, and commit again.
-
CI checks
VS Code extension and pre-commit hooks are great tools to warn developers early and it saves time. However, VS Code extension and pre-commit hooks are not something we can rely on fully. That's because the developer can skip these if they prefer to.
Here are some cases where things might not work out:
- Lint checks from both the pre-commit hook and VS Code extension won't run if the developer forgets to run yarn install while setting up the repository. This is because the installation and setting up of all the npm packages in a repository is done by yarn install command. So, the packages eslint, husky, and lint-staged won't be available to run until yarn install is run.
- Developers can manually skip Git's pre-commit hooks by running git commit --no-verify. The --no-verify flag indicates that I want to commit the changes, but I don't want the hooks to run and block me.
To ensure that the code that goes into the repository is lint-free, we need to implement continuous integration (CI) checks. With CI checks in place, the PR (Pull Request) reviewer can know whether a PR meets the quality standards just by looking into this section of the PR:
Passing CI checks
Failing CI checks
From the CI checks, we will run ESLint without the --fix flag. The goal of CI checks is to detect and report lints and not correct them.
For CI, currently, we are using an in-house developed tool named NeetoCI.
Challenges we faced
neeto is building a lot of products. All these products had a lot of code written in a non-standard way. It meant that when we got started with the code standardization task we had to deal with a large amount of code to fix.
Some of the non-standard codes were auto-correctable. For such cases, we published ESLint rules with auto-fixer. Whenever anyone makes any changes in a file containing that non-standard code pattern, it would get auto-corrected during the pre-commit hook execution. It was easy.
But some cases weren't auto-fixable. For such patterns, we published the ESLint rule and then asked a few engineers to go through all projects and run the command eslint app/**/*.{js,jsx,json}. It would reveal all pieces of code that violate the new rule. Those errors had to be fixed manually. We named this process rollout.
While doing this, we realized that we were not thinking much about false positive and true negative cases. We started encountering them a lot during the rollout. It forced us to make fixes, publish the updates again to npm, and redo the rollout. This was inefficient.
After facing such incidents, we decided to clone all repos locally and test the changes in all products before raising PR. Even though it was hard at the beginning, it proved to be quite effective in detecting all possible edge cases of a rule.
Later on, we started building rules for more complex and abstract standards. It was impossible to detect and flag all non-standard code patterns for them. If we stressed covering more cases, we would start getting several false positives along with it.
A good example would be the hard-coded-strings-should-be-localized rule. The aim was to force people to use i18next based localization instead of hardcoding strings in English. In other words, all the strings that are supposed to be rendered on the DOM should come from the translation files.
If we were to flag all string literals as errors in the code, we would have more false positives than real errors. There were several strings like enum keys that were used only in the application logic. They will never be rendered on UI. There is no point in applying localization to them.
As you might guess, there is no way an ESLint plugin could tell if a string is going to be rendered in the UI or if it is used only in the application logic. To circumvent this, we decided to raise an error if the string contains a space character in it. Since enum keys didn't contain spaces, it eliminated a lot of false positives. But, it created a lot of true negatives. We were missing all one-word strings that were rendered in the UI.
Also, even with that change, we didn't fully eliminate false positives. There were several cases where we used space-separated strings in the application logic. An example is classNames prop. It usually contains a space-separated string of CSS classes (like classNames="flex justify-center").
To minimize such edge cases, we added a whitelist for property names like classNames which should always be ignored, and a blacklist for properties like label which should always be flagged even if it is a single word. With that change, we were able to lower the false positives and true negatives to a great extent.
Even with all this logic in place, we weren't confident that we have eliminated false positives fully. So we decided to publish this rule as a warning. ESLint warnings are less stricter than errors. VS Code extension would still show yellow squiggly lines and we would still get warning statements while running pre-commit hooks. But the pre-commit hook won't fail when a warning is encountered. The same applies to the CI checks.
Lessons learned
In the case of ESLint rules, false positives are more harmful than true negatives. If a false positive error is raised, it would confuse the developers. Even though they are writing code in the right way, the plugin would complain that it is an error. Some developers might even write some tricky code to circumvent the ESLint error thinking that it is genuine.
We always try to minimize false positives by testing the rule with the existing code in several repos before publishing it. In some cases, we can't avoid very rare false positives to favor a large number of true negatives. If a developer encounters false positives for such rules, they are advised to disable the rule for that specific line of code.
Building ESLint rules not only helped us improve the code quality but also gave us exposure to AST parsing and manipulation. With the help of that new knowledge, we were able to accomplish several other achievements. As an example, we have built a custom Babel plugin that could generate the boilerplate code of fetching values from a Zustand store at compile time. You can read about how you can build your own ESLint and Babel plugins on our upcoming blogs. Stay tuned.