Common TypeScript Issues Nº 2: non-empty statements

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.

If you install SonarLint in your editor and copy and paste the example code below you can see these issues for yourself. This problem also occurs in JavaScript projects, TypeScript isn't doing anything strange to cause this.

Nº 2: non-empty statements should change control flow or have at least one side-effect

Let's break the rule down. A non-empty statement is any statement in TypeScript or JavaScript that contains more than just a semicolon. An empty statement looks like this:

;

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:

Comparison

cool == “TypeScript”;

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:

cool = “TypeScript”;

Property access

function submitForm(event: SubmitEvent) {
  event.preventDefault;
  // submission code
}

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:

function submitForm(event: SubmitEvent) {
  event.preventDefault();
  // submission code
}

Missing a return statement

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:

const theThing = "the thing";
const array = ["not the thing", "the thing", "another thing"];

const found = array.find((item) => {
  item === theThing;
});

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 return:

const found = array.find((item) => {
  return item === theThing;
});

or by removing the braces and turning the function into an arrow function expression:

const found = array.find(item => item === theThing);

SonarLint has a special rule for array methods like this to ensure that functions return a value and avoid this bug.

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:

function HelloWorld() {
  <p>Hello world!</p>;
}

Sonar actually has a rule specifically to cover issues like this in React. Fix this by returning your TSX or JSX:

function HelloWorld() {
  return <p>Hello world!</p>;
}

Exceptions to the rule

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.

Keep these exceptions as exceptions

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.

Catch these issues early

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.

What's number 1?

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.

What do you think is going to top this list? Is there a bug or typo your linter always catches you with? Share it with us in the community or with @SonarSource on Twitter.

So far in our Top 5 countdown:

No.3 Unused local variables and functions

No.4 Dropping and creating objects

No.5 Optional property declarations