910722 - JavaScript linting rules are too strict

Status: RESOLVED DUPLICATE of bug 921398

(Firefox OS Graveyard :: General, defect)

Description

[Mike Pennisi [:jugglinmike]]

When working with asynchronous APIs, it is a common practice to create anonymous functions with a bound context. Such functions are most succinctly created as follows:

    var boundAnonymous = function() {
      // callback code
    }.bind(this);

Although this is valid JavaScript, gslint is currently configured to disallow it. Instead, developers must use the grouping operator to return the function expression as an evaluated function object before binding, i.e.:

    var boundAnonymous = (function() {
      // callback code
    }).bind(this);

This unnecessary form tends to make the code base *less* readable because it overloads the meaning of a parenthesis-wrapped function.

When a function is used as a closure that is to be invoked immediately, the language requires it be wrapped in parenthesis:

    var api = (function() {
      // ...
    })();

In this way, the opening parenthesis is a useful indicator that the following function (usually spanning many lines) is going to be invoked immediately.

Forcing developers to use this form for an entirely different pattern makes the code base less readable because it dilutes the meaning of `(function`.

Comment 1

[Mike Pennisi [:jugglinmike]]

Attachment: 910722-lint-rules.patch

Comment 2

[Mike Pennisi [:jugglinmike]]

Attachment: 910722-lint-rules-v2.patch

I just noticed that I neglected to update the githook in my initial patch. Sorry for the noise!

Comment 3

[Anthony Ricaud (:rik)]

Comment on attachment 797279 [details] [diff] [review]
910722-lint-rules-v2.patch

Review of attachment 797279 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is ok for me. But I'll ask a review from a Gaia peer to decide if we want this change in coding style.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 797279 [details] [diff] [review]
910722-lint-rules-v2.patch

Your argument make sense and I don't have a strong opinion about it. Thanks!

Comment 5

[Mike Pennisi [:jugglinmike]]

master: 5db143d5aa532aacbc1522c885ad38849ebd20b9

Thanks for the review, fellas :)

Comment 6

[Anthony Ricaud (:rik)]

Reading closure-linter source code, it looks like this will stop reporting missing semi-colons in expressions like:
var foo = function() {}

If that's the case, I think we should back this out and open an issue in closure_linter to give us an option to just disable this.

Comment 7

[Mike Pennisi [:jugglinmike]]

(In reply to Anthony Ricaud (:rik) from comment #6)
> Reading closure-linter source code, it looks like this will stop reporting
> missing semi-colons in expressions like:
> var foo = function() {}
> 
> If that's the case, I think we should back this out and open an issue in
> closure_linter to give us an option to just disable this.

You are correct. I will back the patch out.

Comment 8

[Mike Pennisi [:jugglinmike]]

backed out: https://github.com/mozilla-b2g/gaia/commit/e5baba667e8c37b7f179483406f832b89ebc75d2

Comment 9

[Anthony Ricaud (:rik)]

Duping this bug since 921398 as a link to a closure_linter bug.