Add rules for prohibiting unused vars/expressions

[haroldtreen]

Add no-unused-* rules

Status: Ready for Review Reviewers: @jansepar @donnielrt @marlowpayne

Changes:

• Add rules for no-unused-variables and no-unused-expressions

I have found these rules especially useful when they detect imports I don't use: eg.

define(['somethingUsed', 'somethingUnused'], function(used, unused) { // Linting error!
      used();
});

// OR

var used = require('used');
var unused = require('unused'); // Linting error!

used();

[jansepar]

3am commits to the code style repo. Welcome fellow night time coder :)

:+1 thx Harold!

[marlowpayne]

Dang you guys be crazy with late night commits.

I'm wondering if these rules would break a common convention we sometimes use where we require in, but don't end up using the var for anything except side effects. For example selector-utils, or Deckard:

define([
    '$',
    'fastclick',
    'deckard'
],
function(
    $,
    fastclick
) {
    var globalUI = function() {
...

Also, these new rules should probably be run against a newly generated Adaptive project to check if anything needs updating in the generator to conform to the new ruleset.

[jansepar]

For the ones we bring in just for side effects, we shouldn't make an argument variable for it, and if we stick to that the linter should be fine. Like in you example above, we have no argument in the callback for deckard.

+1 to testing it in a newly generated project!

[haroldtreen]

Who can sleep when there may be unused variables in the wild! Testing on a generated project sounds good! I shall do that.

As for side effect imports, Shawn's suggestion would work. I also wonder if the pattern of importing things to change the state of the world is good. Is there a way to make it more explicit that those packages are needed and do stuff?

Eg.

define([
    '$',
    'fastclick',
    'deckard'
], function($, fastclick, loadDeckard) { 
     loadDeckard();
});

Probably not a backwards compatible change, but I wonder if that would be a better system for future plugins.

[jansepar]

Totally - I think as much as possible libraries should be built as modules that are side-effect less until invoked. We should change this for Deckard, as we have done for many libraries of the past, such as capturing, the Mobify Tag, etc. Unfortunately we don't own every library we use, so for those that don't conform to a module system, we will have to just ensure we don't put argument variables for them to conform to the style guide. On Sat, Nov 14, 2015 at 11:53 AM Harold Treen [email protected] wrote:

Who can sleep when there may be unused variables in the wild! Testing on a generated project sounds good! I shall do that.

As for side effect imports, Shawn's suggestion would work. I also wonder if the pattern of importing things to change the state of the world is good. Is there a way to make it more explicit that those packages are needed and do stuff?

Eg.

define([ '$', 'fastclick', 'deckard'

], function($, fastclick, loadDeckard) { loadDeckard(); });

Probably not a backwards compatible change, but I wonder if that would be a better system for future plugins.

— Reply to this email directly or view it on GitHub https://github.com/mobify/mobify-code-style/pull/78#issuecomment-156742288 .

[benlast]

:+1: to using eslint to enforce this and related issues. We've found it very useful on Pusheen.

[marlowpayne]

Even with options passed to both rules, a newly generated Adaptive.js project has JS lints. We should investigate what we need to change in our boilerplate code before we merge this.