Configure our specific needs for JavaScript style checking

[Robbert]

• Single quotes or double quotes?
• We need to decide on what line the curly braces go.
• Allow the "double equals shortcut" foo == null for foo === null || foo === undefined. (Although ultimately, I prefer the explicit version when the Closure Compiler optimizes this and combines the two === comparisons into one.)
• Allow double equals for comparing against type string: typeof foo == "string". This might require writing an eslint plugin.

There are a couple of eslint rules I initally wanted to use but prove problematic:

• no-multi-spaces results in a lot of warnings, because I like to align similar equals statements and variable assignments. For example:

return uri.path      === base.path
    && uri.authority === base.authority
    && uri.query     === base.query
    && uri.scheme    === base.scheme;

• no-unused-expressions results in many, many errors because I use empty property reference to annotate them for the Closure Compiler. For example:

/** @type {Array.<string>} */
URI.prototype.dirs;

Fortunately the Closure Compiler also supports detection of unused expressions, so it isn't a blocking issue, but during development it would still be great to have eslint errors for unused code inside Sublime.

• no-magic-numbers results in an incredible amount of warnings, especially for references to array and string indexes, which are completely valid. For example: arr[arr.length - 1]
• no-unsafe-finally results in the error Definition for rule 'no-unsafe-finally' was not found -- perhaps it is a feature that hasn't been released yet.
• no-implicit-globals results in warnings for var declarations in Node.js scripts. This rule should only apply to AMD modules and browser scripts.
• no-extra-parens warns for "Gratuitous parentheses around expression" for JSDoc type casting workarounds for the Closure Compiler, and of course rightfully so, it is a strange and undesirable workaround... For example:

var match = /** @type {Array.<string>} */ (URI.regexp1.exec(string));

• no-self-assign is also causing problems with these JSDoc type casting workarounds...

e = /** @type {Error} */ (e);

• no-inline-comments is also triggered for type cast annotations:

e = /** @type {Error} */ (e);

I would like to tweak the implementation of this rule to recognize JSDoc comments.

• no-unmodified-loop-condition is causing warnings for this situation because l is unmodified, but this is a perfectly valid use case (in unit tests).

var n = 0, l = 1E5;
for (; n < l; n += 2) {
}

• valid-typeof doesn't allow typeof arg === 'unknown', which is returned by Internet Explorer 6-8. Guess we need to fix eslint to support 'unknown'.
• var undefined = null; and function undefined() {} should not be allowed. Comments by @Yolijn pointed out this wasn't being checked for by eslint currently. Must prevent variable assignments

[Robbert]

I think it would be good to configure all settings explicitly, even when we use the same value as the implicit default.

• This better conveys our intent.
• Gives an opportunity to detect new rules we haven't yet given consideration.
• Can be used as checklist when considering a new linting tool.

[Yolijn]

I suggest we try to use an eslint rule for overwriting the no-unused-expressions check. This way we could get warnings during development (in Sublime) but not for commit checks.

[Robbert]

I'm leaning towards single quotes for JavaScript and CSS, double quotes for HTML.

Reasons:

• I think the single quote variant is more readable, especially at a glance:

var href = explicit ? "" : ".";
var snippet = '<a href="?q=test">hello world</a>';

var href = explicit ? '' : '.';
var snippet = '<a href="?q=test">hello world</a>';

• Consistency when embedding XML and HTML snippets
• The cool kids do it! (lodash uses single quotes)
• Most Node.js projects use single quotes.
• Less strain when typing, no Shift+' needed.
• I always preferred using quotes when working outside of wealth/src/

@Yolijn Decision day!

[Robbert]

Just found out I can convert the entire codebase to single quotes with this oneliner on the command line:

eslint --fix --no-eslintrc --rule "quotes: ['error', 'single', { 'avoidEscape': true }]" src/

[Robbert]

Single quotes it is! Jay.

[Robbert]

Curly braces on the same line or the next line?

function (a) {
    if (a) {
        return 1;
    }
    else {
        return -1;
    }
}

Or:

function (a)
{
    if (a)
    {
        return 1;
    }
    else
    {
        return -1;
    }
}

Or:

function (a)
{
    if (a) {
        return 1;
    } else {
        return -1;
    }
}

Or:

function (a)
{
    if (a) {
        return 1;
    }
    else {
        return -1;
    }
}

Or:

function (a) {
    if (a) {
        return 1;
    }
    else {
        return -1;
    }
}

Example scripts:

• lodash
• jQuery
• Closure Library

Observations:

• lodash inconsistently uses } else { and else {
• Closure Library and jQuery consistently use } else {
• same line for function declarations: jQuery, lodash, Closure Compiler
• jQuery frequently uses an empty line on the first line of an if block that is followed by else, or sometimes starts the else block with an empty line increase visual distinction between the distinct code blocks.

[Robbert]

Regarding the eqeqeq rule:

• I don't like using triple equals for typeof when the other side of the comparison is a string constant. Since this language construct is guaranteed to have string types on both sides, no type coercion will ever occur. typeof a === "string" doesn't look elegant to me, typeof a == "string" seems slightly better.
• Sometimes I implement the SomeConstructor.prototype.valueOf method to aid comparisons by allowing type coercion. Take for example QName:

// Before implementing `valueOf`:
if (xslTemplate.qName.namespaceURI === "http://www.w3.org/1999/xhtml" &&
    xslTemplate.qName.name === "script") {
}

// After implementing `valueOf`:
if (xslTemplate.qName == "{http://www.w3.org/1999/xhtml}script") {
}

• I want to use === everywhere else.

Regarding the option allow-null:

• We use obj.namespaceURI === null quite a lot, and it might be hard to distinguish the accidental arg == null from the purposely used arg == null.
• Therefore, it might be desirable to use arg == undefined instead for those cases.
• But because undefined can be redefined using var undefined = true for example, we'd better not use arg == undefined.
• All in all, it might be best to explicitly use arg === null || arg === undefined and leave it to the Closure Compiler or another minifier to optimize and use the compact version.

[Robbert]

I've forked and tweaked eslint to support configuring custom types in valid-typeof.js.

When you use npm link to use this fork you can now use:

{
  "rules": {
    "valid-typeof": ["error", { "allow": ["unknown"] }]
  }
}

This way typeof foo !== "unknown" won't result in an error.

• fork'n'fix
• Update documentation in valid-typeof.md to include the configuration option.
• Sign CLA.
• Make a pull request.

[Robbert]

I have disabled the no-control-regex option to allow implementing expressions such as:

Character.NON_LOWER_ASCII = /[^\x00-\x7f]/g

The use case for this test is of course accidental uses of control characters, and it seems that it is all too easy to put an escape slash in front of a zero: 0 -> \0. Perhaps it is wise to disallow this one, and explicitly require the more verbose \x00.

• disallow \0 (in regular expressions)

[Robbert]

With great power comes great responsibility: I'm moving the rule: ["off"] for a couple of checks to the specialized src/.eslintrc.json. This means that some things are only allowed in library code, and not in application code.

For example:

• no-control-regex: we sometimes need \x00
• no-new-fun: we sometimes need eval()

Also, because we need to prevent minifying of properties sometimes:

• dot-notation is required by default, but disabled for src/

[Robbert]

I guess we should have the setup for the test/ directory, where we can disable some rules because we want our code to work in edge cases or test for XSS vulnerabilities.

• no-new-wrappers
• disallowing "javscript:" URLs

[Robbert]

These are best left to Closure Compiler:

• array-callback-return

The following are ES6 rules and need to be defined when we start using ES6. Currently set to off:

• arrow-body-style

Some rules are disabled because they are taken care off by other rules:

• block-scoped-var is implied by vars-on-top

[Robbert]

Regarding init-rule-declaration, I would want to enforce declarations for primitive literals, but not for more memory heavy values like objects and arrays.

Bad:

var index, length, obj,
    array = [];
index = 0;
length = array.length;

if (enabled) {
    obj = {};
}

Good:

var obj,
    index = 0,
    length = array.length,
    array = [];

if (enabled) {
    obj = {};
}

This is not possible with eslint currently, but we could implement it.

[Robbert]

The should be warnings in wealth/ and errors in frameless/:

"lines-around-comment": [2, { "allowBlockEnd": true } is causing a lot of errors for unfinished commented out code after a return value:

function ()
{
    ...

    return output;

    // TODO: Finish implementation
    /*
    for (var ...
    */
}

Of course we shouldn't allow this in the high-quality repository, but we should enable warnings for this in the meantime.

[Robbert]

no-dupe-args is desirable, but I like using duplicate vars for unused variables in callback functions for String.prototype.replace:

regexp.replace(function ()_, firstName, _, _, lastName) {
});

[Robbert]

"operator-linebreak": ["error", "before"] currently seems to require all binary operators to be spread accross lines. Of course this is highly undesirable. We need to tweak the implementation to test for situations on which the left-hand side and right-hand side are on different lines, and only then report the situation.

• Re-enable max-statements-per-line when the an "as-needed" or "consistent" option has been implemented

[Robbert]

max-statements-per-line is buggy for { "max": 1 }.

• Re-enable "max-statements-per-line": { "max": 1 } when the bug has been fixed.

[Robbert]

• new-cap warns for CollectGarbage() which is a native function that is constructor-cased. We should add an exception for this one.

[Robbert]

Related reading:

1. https://engineering.mongodb.com/post/succeeding-with-clangformat-part-1-pitfalls-and-planning/
2. https://engineering.mongodb.com/post/succeeding-with-clangformat-part-2-the-big-reformat/