Use ESLint to enforce code quality

[queengooborg]

Derived from #4074, which initially discussed both ESLint and Prettier. This issue discusses specifically ESLint.

Now that we're enforcing the code styling with Prettier (well, in the process of enforcing it at the time of this writing), we should also start adding linting for code quality, which is where ESLint comes in. It was proposed in mdn/mdn#71 that ESLint should supplement Prettier by disabling all of the code styling rules (which Prettier has an ESLint plugin for).

I've got a branch ready to send as a pull request (using ESLint with the JavaScript Standard formatting), if this is something to pursue!

[queengooborg]

#5635 was closed as some initial discussion was desired beforehand, regarding the rules to implement. In my branch, I had the following series of rules:

1. Enforce Unix-style linebreaks to maintain consistency in case Windows users don't enable auto-CRLF
2. Enforce single-quotes and always end lines with semicolons
3. Disallow empty block statements aside from catches in try-catch
4. Enforce Prettier's rules, since there's an ESLint plugin to connect ESLint and Prettier
5. Warn about JSDoc issues (only warn, not error, since JSDocs aren't a big concern for us)
6. Enforce arrow function syntax for consistency (most of our functions are in arrow syntax, so I think it makes sense to keep them all arrow functions)

[ddbeck]

Thanks for bringing the discussion back here, @vinyldarkscratch. I'll respond to your comment in more detail below, but first:

For my part, some questions I want to answer before adding ESLint are:

• What code quality problems do we have now?
• Does ESLint find those problems? Do we have to configure is specially to find those things, or is it from a default?
• What problems can ESLint fix? What problems do we need to put some time into fixing?

Overall, I'd be comfortable adding ESLint to the project when I know what problems it will solve, how it will solve them, and what work is needed to make passing ESLint a requirement to merge.

OK, getting to your specific points now.

#5635 was closed as some initial discussion was desired beforehand, regarding the rules to implement. In my branch, I had the following series of rules:

In general, I'm interested in knowing more about how you got to this set of rules? Did you have a list of problems you knew of in advance?

1. Enforce Unix-style linebreaks to maintain consistency in case Windows users don't enable auto-CRLF

Is this something we can fix with .gitattributes (e.g., *.js text eol=lf)? This sounds reminiscent of #3231.

2. Enforce single-quotes and always end lines with semicolons
4. Enforce Prettier's rules, since there's an ESLint plugin to connect ESLint and Prettier

(grouping two points here)

I'm confused about the relationship between ESLint and prettier. The prettier docs seem to suggest turning off ESLint's code style rules and letting prettier alone enforce style. But here you seem to be talking about using ESLint to enforce prettier's style. What are the pros and cons of the two approaches?

3. Disallow empty block statements aside from catches in try-catch

Are there examples of this in our code base? A pointer to this would be helpful, to show a concrete thing that ESLint might fix.

5. Warn about JSDoc issues (only warn, not error, since JSDocs aren't a big concern for us)

What are JSDoc issues? If we don't error on those issues, is there a way to check these problems incrementally? If I'm editing some code, is there a way to hide errors that don't have to do with my changes?

6. Enforce arrow function syntax for consistency (most of our functions are in arrow syntax, so I think it makes sense to keep them all arrow functions)

It'll be interesting to see what the diff is on this! I've got mixed feelings about the readability of arrow functions, though I like writing them.

[queengooborg]

Happy to answer those questions!

---

• What code quality problems do we have now?
• Does ESLint find those problems? Do we have to configure is specially to find those things, or is it from a default?

Code quality problems that we currently have within our code that ESLint catches by default are the following:

• Unused variables
• Nested functions
• Attempts to reassign const variables
• References to invalid variables (not currently, but there have been some in the past)
• Control characters in regex
• Missing break statements in switch statements

There are other code quality issues that ESLint catches by default that we haven't had an issue with so far. There are additional rules we can enable to enforce best code quality practices (such as disallowing empty functions, or the use of eval(), or the use of the arguments variable in functions) which can be enabled to our liking. A full list of ESLint's built-in rules can be found here: https://eslint.org/docs/rules/ (rules with a ✔️are enabled by default, and rules with a 🔧are automatically fixable)

---

• What problems can ESLint fix? What problems do we need to put some time into fixing?

None of the problems we currently have are automatically fixable by ESLint and require manual review, but it can at least point us towards those errors that we may not catch during our review.

---

1. Enforce Unix-style linebreaks to maintain consistency in case Windows users don't enable auto-CRLF

Is this something we can fix with .gitattributes (e.g., *.js text eol=lf)? This sounds reminiscent of #3231.

I believe we can, yeah! During my review, I was also playing around with my Windows system to try it out, and realized it creates more problems than solves them (I remembered it backwards as well: actually auto-CRLF is what makes line endings Windows-style rather than Unix-style, durr). Using .gitattributes will be much more effective!

---

2. Enforce single-quotes and always end lines with semicolons
3. Enforce Prettier's rules, since there's an ESLint plugin to connect ESLint and Prettier

(grouping two points here)

I'm confused about the relationship between ESLint and prettier. The prettier docs seem to suggest turning off ESLint's code style rules and letting prettier alone enforce style. But here you seem to be talking about using ESLint to enforce prettier's style. What are the pros and cons of the two approaches?

In regards to point 2, if I remember correctly, I ran into a few issues with single-quotes and semicolons not being enforced properly initially. After testing again just now, the semicolons rule wasn't needed in ESLint, but there's a few quotations that ESLint catches and Prettier doesn't (i.e. using " ` " instead of " ' " for strings without interpolation).

In regards to point 4, and what you were reading about in the docs, the ESLint Prettier plugin integrates Prettier into ESLint to allow the two to function in harmony. First, it disables all styling rules provided by ESLint, then runs Prettier and reports the results back to ESLint. This way, we only get one stream of errors, rather than two which may have duplicates. So far I haven't seen any cons to the ESLint + Prettier approach.

---

3. Disallow empty block statements aside from catches in try-catch

Are there examples of this in our code base? A pointer to this would be helpful, to show a concrete thing that ESLint might fix.

I don't recall seeing this within our code to be entirely honest, but this rule's an ESLint default. So, in other words, we wouldn't ever have to see this in our code!

---

5. Warn about JSDoc issues (only warn, not error, since JSDocs aren't a big concern for us)

What are JSDoc issues? If we don't error on those issues, is there a way to check these problems incrementally? If I'm editing some code, is there a way to hide errors that don't have to do with my changes?

The most common issues we've got with our JSDocs right now are missing descriptions to describe functions, its parameters, and its return values. Others include the misuse of typenames within JSDoc (specifically assumptions that all TypeScript variables and names are allowed by JSDoc), or the inconsistent use of JSDoc tags by their true name or their nickname.

Unfortunately I don't think there's a way to check these problems incrementally, hence why I chose to set these as warnings. I don't believe that the JSDocs are much of a concern of ours; they're more like a P5-type thing. I certainly feel that having documentation on which linter functions do what will help us in the future, though.

---

6. Enforce arrow function syntax for consistency (most of our functions are in arrow syntax, so I think it makes sense to keep them all arrow functions)

It'll be interesting to see what the diff is on this! I've got mixed feelings about the readability of arrow functions, though I like writing them.

Same here, there's something quite satisfying about using an arrow to define a function! Admittedly, of course, this is more of a personal preference rather than a functionality issue, but I do think we should choose one style and stick with it for legibility purposes.

---

Hope this helps, and I'm happy to answer any additional questions or include additional details!

[ddbeck]

OK, that's really helpful, @vinyldarkscratch. I guess the next step from here would be to break this down into small, manageable PRs. One route might be:

1. Open a PR that adds ESLint with its default settings. This PR would fail the build and would not be merged until all the default problems are fixed. Effectively, this would be the tracking issue for the various subtasks presented by ESLint's rules.
2. Open PRs to resolve, one by one, the problems identified by ESLint. Given limited time to review these things, keeping the diffs small enough to be reviewed in minutes is key to making any real progress on this (similarly, I'd like to keep the number of open PRs to a minimum, since data stuff takes priority). I'd expect this to go by rule or module or both.
3. After we've introduced ESLint with its defaults, we could then repeat the process with changes to ESLint's default config, such as extra rules for JSDoc or arrow functions.

[queengooborg]

ESLint has been added a while back, so this issue can now be closed!