tools: enable array-callback-return ESLint rule #17858

Trott · 2017-12-25T07:17:48Z

For array methods that depend on a callback (such as .filter() or
.map()), require a return value from the callback.

First two commits fix a few issues in our code base that would be flagged by this rule.

Third commit enables the rule itself.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools readline test

TimothyGu · 2017-12-25T07:48:45Z

lib/readline.js

@@ -501,7 +501,7 @@ Interface.prototype._tabComplete = function(lastKeypressWasTab) {

      // If there is a common prefix to all matches, then apply that portion.
      var f = completions.filter(function completionFilter(e) {


this could be reduced to completions.filter(e => e).

👍

I was being cautious. MDN indicates that the callback should return true or false and makes no mention of truthy or falsy values. But now I've looked at the ECMA-262 spec and it makes it explicit that the callback may return a value that is coercible to a Boolean. I'll change it to your suggestion to minimize the unnecessary changes here.

(Might not bother with the change to arrow function just to keep the diff as small as possible. I assume there's no strong preference there.)

I’d go for it. +1 -3 vs. +1 -1 are both really small, and I don’t think this line is interesting enough for people to use git blame on it either way.

starkwang · 2017-12-25T07:48:18Z

lib/readline.js

@@ -501,7 +501,7 @@ Interface.prototype._tabComplete = function(lastKeypressWasTab) {

      // If there is a common prefix to all matches, then apply that portion.
      var f = completions.filter(function completionFilter(e) {
-        if (e) return e;
+        return !!e;


Nit: using Boolean(e) here might be more readable

Use construct that always returns a boolean for `filter()` callback.

Two tests were using Array.prototype.map() without returning values in the callback. In other words, they were using it where a `.forEach()` was called for. Change to `.forEach()`.

For array methods that depend on a callback (such as `.filter()` or `.map()`), require a return value from the callback.

Trott · 2017-12-27T19:55:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12323/

jasnell · 2017-12-28T17:04:46Z

CI failures look unrelated.

Use construct that always returns a boolean for `filter()` callback. PR-URL: #17858 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Two tests were using Array.prototype.map() without returning values in the callback. In other words, they were using it where a `.forEach()` was called for. Change to `.forEach()`. PR-URL: #17858 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

For array methods that depend on a callback (such as `.filter()` or `.map()`), require a return value from the callback. PR-URL: #17858 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell · 2017-12-28T17:09:53Z

Landed in 5c7af90, fe8a5aa, and a3535f3