The Depths Of ES-Lint

Not long before, in my usual quest of discovering new Open Source Repositories, and finding somewhere I can learn, improve and contribute to, I came across treeherder.

Treeherder – A system for managing CI data for Mozilla projects.

A repository, that could be overwhelming for someone to contribute to, especially if you’re getting started with Open Source.

Luckily, there were some issues that needed help! That’s where I enter!

TL;DR – ES-Lint is a pluggable linting utility for JavaScript and JSX.

ESLint is a tool for identifying and reporting on patterns found in ECMAScript/JavaScript code, with the goal of making code more consistent and avoiding bugs. In many ways, it is similar to JSLint and JSHint with a few exceptions:

  • ESLint uses Espree for JavaScript parsing.
  • ESLint uses an AST to evaluate patterns in code.
  • ESLint is completely pluggable, every single rule is a plugin and you can add more at runtime.

To find more about ES-Lint – https://eslint.org/docs/user-guide/getting-started

So this was the issue
Re-enable few linting rules – batch #1 

Remove these lines:

    'class-methods-use-this': 'off',
    'consistent-return': 'off',
    'default-case': 'off',

and make sure yarn lint passes before opening a PR.

Removing those lines from lint file is a pretty rookie task, but making all the test cases pass on yarn lint – Oh Boy, with 50 test cases failing after removing those lines, I knew this would take a while.

The first step, like always, was, to read the documentation for what is being done by which rule. Let’s go through this step-by-step.

Class Method Use This

Enforce that class methods utilize this (class-methods-use-this)

If a class method does not use this, it can sometimes be made into a static function. If you do convert the method into a static function, instances of the class that call that particular method have to be converted to a static call as well (MyClass.callStaticMethod())

It’s possible to have a class method which doesn’t use this, such as:

class A {
    constructor() {
        this.a = "hi";
    }

    print() {
        console.log(this.a);
    }

    sayHi() {
        console.log("hi");
    }
}

let a = new A();
a.sayHi(); // => "hi"

In the example above, the sayHi method doesn’t use this, so we can make it a static method:

class A {
    constructor() {
        this.a = "hi";
    }

    print() {
        console.log(this.a);
    }

    static sayHi() {
        console.log("hi");
    }
}

A.sayHi(); // => "hi"

There were a total of 11 instances in the code where I found a method inside a class that didn’t use the this keyword. And, it wasn’t the most to figure out how to go about changing these, because some of them just didn’t have any properties.

To literally pass some test cases:
I included the line:

this.forThis = null; // Just adding to use this

Nevertheless, I could somehow make all those changes and get the lint tests to pass!

Consistent Return

require return statements to either always or never specify values (consistent-return)

Unlike statically-typed languages which enforce that a function returns a specified type of value, JavaScript allows different code paths in a function to return different types of values.

A confusing aspect of JavaScript is that a function returns undefined if any of the following are true:

  • it does not execute a return statement before it exits
  • it executes return which does not specify a value explicitly
  • it executes return undefined
  • it executes return void followed by an expression (for example, a function call)
  • it executes return followed by any other expression which evaluates to undefined

If any code paths in a function return a value explicitly but some code path do not return a value explicitly, it might be a typing mistake, especially in a large function. In the following example:

  • a code path through the function returns a Boolean value true
  • another code path does not return a value explicitly, therefore returns undefined implicitly
function doSomething(condition) {
    if (condition) {
        return true;
    } else {
        return;
    }
}

Rule Details

This rule requires return statements to either always or never specify values. This rule ignores function definitions where the name begins with an uppercase letter, because constructors (when invoked with the new operator) return the instantiated object implicitly if they do not return another object explicitly.

Examples of incorrect code for this rule:

/*eslint consistent-return: "error"*/

function doSomething(condition) {
    if (condition) {
        return true;
    } else {
        return;
    }
}

function doSomething(condition) {
    if (condition) {
        return true;
    }
}

Examples of correct code for this rule:

/*eslint consistent-return: "error"*/

function doSomething(condition) {
    if (condition) {
        return true;
    } else {
        return false;
    }
}

function Foo() {
    if (!(this instanceof Foo)) {
        return new Foo();
    }

    this.a = 0;
}

So, in accordance with this rule, all methods and functions should consistently return or not return a value at the end of the method or function!

This is exactly where the issue got overwhelming!

A whooping 39 test cases failed after turning on consistent return.
Now you can’t just figure out what a function might be returning, obviously, not until you read the entire function properly, and read what it does.

Well, I did that, and added the required return statements wherever I could.

But still, to make this thing work, in some cases, I just wrote,

return null

Well, that did the trick.

Onto the next rule.

Default Case

Require Default Case in Switch Statements (default-case)

Some code conventions require that all switch statements have a default case, even if the default case is empty, such as:

switch (foo) {
    case 1:
        doSomething();
        break;

    case 2:
        doSomething();
        break;

    default:
        // do nothing
}

The thinking is that it’s better to always explicitly state what the default behavior should be so that it’s clear whether or not the developer forgot to include the default behavior by mistake.

Other code conventions allow you to skip the default case so long as there is a comment indicating the omission is intentional, such as:

switch (foo) {
    case 1:
        doSomething();
        break;

    case 2:
        doSomething();
        break;

    // no default
}

Once again, the intent here is to show that the developer intended for there to be no default behavior.

Rule Details

This rule aims to require default case in switch statements. You may optionally include a // no default after the last case if there is no default case. The comment may be in any desired case, such as // No Default.

Examples of incorrect code for this rule:

/*eslint default-case: "error"*/

switch (a) {
    case 1:
        /* code */
        break;
}


Examples of correct code for this rule:

/*eslint default-case: "error"*/

switch (a) {
    case 1:
        /* code */
        break;

    default:
        /* code */
        break;
}


switch (a) {
    case 1:
        /* code */
        break;

    // no default
}

switch (a) {
    case 1:
        /* code */
        break;

    // No Default
}

So, the easiest of the three, this rule says every switch statement must have a default case in the end.
Only 4 test cases failed on enabling this rule, and just adding a default case, even with no code did the trick on this one.

I knew that there are multiple places where I had done some things to just make the test cases pass, and I almost felt a little funny when creating a PR for this issue, but turns out:

I did good! YAY!

To my surprise, another one of those amazing mentors out there came, and reviewed my code again!

Yeah, this is good stuff!

Anyways, there’s still a lot left to do with this issue, and I’ll possible create a part two of this blog when the patch I created is merged!

What I learned – so far –

  1. How to enable and disable rules in ES Lint
  2. Why ES Lint rules are so important
  3. After enabling each rule, reading it’s documentation and understanding what it really does
  4. Making the required changes so that the test cases still pass!

I liked working on this issue, it was nice!

Written with love,
Rishabh Budhiraja

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s