Issues crop up in our TypeScript code all the time, but many are solved immediately with tooling like SonarLint and its rules designed to catch them. We're counting down the top 5 issues that SonarLint spots in all of our TypeScript.
We're not ruling a lot out at this stage. So what should a non-empty statement do? It should either change control flow – it should either branch, loop, break a loop, or throw/catch an error–or it should have a side-effect–literally it should do something. So a statement that would fail this rule effectively does nothing. Even though code exists, the system doesn't change as a result of running the line of code. Some examples of this include:
While TypeScript might well be cool, the statement above doesn't do anything. This normally happens when you intend to assign something to a variable, but typo an extra
= and render the statement useless. Fix it by removing the errant
= and turning the statement back into an assignment:
In this case the first line of the function only accesses the
preventDefault property of the
event object, it doesn’t actually call the function. So the event will continue as normal. This is a case of missing parentheses, another easy mistake to make. Fix it by adding those parentheses:
This can manifest the same way as the comparison example above, but in this case the intention wasn’t to make an assignment, but return a boolean value:
In this case the comparison does nothing and the
find operation never finds anything because the return value from each run of the function is
undefined. You can fix this by using
or by removing the braces and turning the function into an arrow function expression:
The lack of a return statement rears its head quite commonly in React applications too. Failing to return from a
render function or from a functional component will mean nothing is rendered on the page. This simple component will render nothing:
Sonar actually has a rule specifically to cover issues like this in React. Fix this by returning your TSX or JSX:
While delving through some open source projects to find issues like this, I came across some examples where this rule had to be violated. One example of this is in the venerable jQuery project. This is done to support Internet Explorer, not something on too many developers' minds these days thankfully.
When setting the
selected attribute on an
<option> element, IE requires the
selectedIndex property to be accessed before it will respect the change. In this case the jQuery project was using ESLint to lint their code and had to turn off the similar rule "no-unused-expressions" in order to support IE.
At least now IE is retired, violations like this can be removed, though jQuery does still list IE9+ among its supported browsers.
As a side note, you can write code in your application that triggers behaviour based on property access like this. Either writing a getter or wrapping an object in a Proxy can add side-effects to a property like this. But this is not a good pattern. Much like creating and dropping an object just for the side-effects in the constructor function you shouldn't obfuscate behaviour behind operations that don't normally cause side-effects. The jQuery example needed 4 lines of comments to explain itself and 3 comments disabling ESLint. It's best to avoid situations like this, don't write code that causes side-effects from property access.
Issues like non-empty statements that don’t either change control flow or have at least one side-effect should be caught early, by testing or even earlier by checking your code with a linter like SonarLint.
In this series of blog posts we've seen four of our top 5 common issues that crop up in TypeScript projects and are flagged using SonarLint. Remember that these are issues caught by the tooling, so they mostly don't find their way into committed code, and that's what this tooling is for.
So far in our Top 5 countdown: