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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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

Landed in 5c7af90, fe8a5aa, and a3535f3

Use construct that always returns a boolean for `filter()` callback. PR-URL: #17858 Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Use construct that always returns a boolean for `filter()` callback. PR-URL: #17858 Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Use construct that always returns a boolean for `filter()` callback. PR-URL: #17858 Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

gibfahn · 2018-01-24T02:49:51Z

Assuming this doesn't need to be backported, raise a PR if you disagree.

Trott added readline Issues and PRs related to the built-in readline module. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Dec 25, 2017

nodejs-github-bot added readline Issues and PRs related to the built-in readline module. tools Issues and PRs related to the tools directory. labels Dec 25, 2017

TimothyGu approved these changes Dec 25, 2017

View reviewed changes

starkwang approved these changes Dec 25, 2017

View reviewed changes

Trott force-pushed the return-values branch from c00989f to 12e702b Compare December 25, 2017 16:04

cjihrig approved these changes Dec 25, 2017

View reviewed changes

lpinca approved these changes Dec 25, 2017

View reviewed changes

XadillaX approved these changes Dec 26, 2017

View reviewed changes

jasnell approved these changes Dec 27, 2017

View reviewed changes

Trott added 3 commits December 27, 2017 11:55

readline: refactor filter() callback

d018b7b

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

test: replace map() with forEach() where appropriate

5817019

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()`.

tools: enable array-callback-return ESLint rule

22a5f05

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

Trott force-pushed the return-values branch from 12e702b to 22a5f05 Compare December 27, 2017 19:55

TimothyGu approved these changes Dec 28, 2017

View reviewed changes

BridgeAR approved these changes Dec 28, 2017

View reviewed changes

jasnell closed this Dec 28, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

gibfahn added dont-land-on-v6.x labels Jan 24, 2018

Trott deleted the return-values branch January 13, 2022 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Trott commented Dec 25, 2017

TimothyGu Dec 25, 2017

Trott Dec 25, 2017 •

edited

Loading

Trott Dec 25, 2017

TimothyGu Dec 26, 2017

starkwang Dec 25, 2017

Trott commented Dec 27, 2017

jasnell commented Dec 28, 2017

jasnell commented Dec 28, 2017

gibfahn commented Jan 24, 2018

		@@ -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) {

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

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

Conversation

Trott commented Dec 25, 2017

Checklist

Affected core subsystem(s)

TimothyGu Dec 25, 2017

Choose a reason for hiding this comment

Trott Dec 25, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Dec 25, 2017

Choose a reason for hiding this comment

TimothyGu Dec 26, 2017

Choose a reason for hiding this comment

starkwang Dec 25, 2017

Choose a reason for hiding this comment

Trott commented Dec 27, 2017

jasnell commented Dec 28, 2017

jasnell commented Dec 28, 2017

gibfahn commented Jan 24, 2018

Trott Dec 25, 2017 •

edited

Loading