tools: require function declarations #12711

Trott · 2017-04-28T00:34:00Z

Welp, not sure how this will be received, but since it is the predominant style in our code base and since we are occasionally offering nits to new contributors to use the style to avoid potential confusion over the Temporal Dead Zone....

In lib, doc, and test (but mostly test): Replace function expressions with function declarations in preparation for a lint rule requiring function declarations.

Then: Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations).

Checklist

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

Affected core subsystem(s)

tools test lib doc

hiroppy · 2017-04-28T00:40:14Z

doc/api/util.md

@@ -592,7 +592,7 @@ Returns `true` if the given `object` is a `Function`. Otherwise, returns
 const util = require('util');

 function Foo() {}
-const Bar = function() {};
+const Bar = () => {};


common.noop ?

@abouthiroppy Are you confusing this doc file with a test?

@mscdex Oops, thanks 😓

vsemozhetbyt · 2017-04-28T08:51:56Z

.eslintrc.yaml

@@ -97,6 +97,7 @@ rules:
  eol-last: 2
  func-call-spacing: 2
  func-name-matching: 2
+  func-style: [2, "declaration", {allowArrowFunctions: true}]


Can the quotes be omitted here?

Can the quotes be omitted here?

Yes, indeed. Done!

vsemozhetbyt · 2017-04-28T08:57:55Z

test/addons-napi/test_async/test.js

@@ -9,5 +9,4 @@ test_async.Test(5, common.mustCall(function(err, val) {
  process.nextTick(common.mustCall(function() {}));


Can it also be common.mustCall()?

Can it also be common.mustCall()?

Sure seems like it! Done! Thanks!

vsemozhetbyt · 2017-04-28T09:13:05Z

test/parallel/test-event-emitter-add-listeners.js

@@ -68,8 +68,8 @@ const EventEmitter = require('events');
 }

 {
-  const listen1 = function listen1() {};
-  const listen2 = function listen2() {};
+  const listen1 = () => {};


common.noop?

Not here or on the line below, and this is actually something that I've worried about for a while: common.noop doesn't really save us a lot of code and can lead to subtle bugs such as if it is used here.

If used here and the line below, then listen1 and listen2 are the same object, thus invalidating the test!

This could perhaps be fixed by making common.noop a getter that returns a different no-op function object each time it is called. But I kinda wonder if we might just be better off doing a global find/replace on common.noop and using () => {} instead.

/cc @nodejs/testing

@Trott on the other hand, test/parallel/test-event-emitter-remove-all-listeners relies on common.noop being the same object (discovered that by actually making common.noop a getter).

@aqrln In that case, I imagine explicitly defining const noop = () => {}; in the test itself would make it clear that everything is intentionally using the same function object and that it's not a mistake.

@Trott yeah, makes sense. I will follow up with a PR quickly.

I just simply would not use common.noop here. In fact, if memory serves correctly, when I opened the common.noop PR just a few weeks ago I intentionally left this test alone because it did not make sense to use it here... and that is perfectly fine. It's ok not to use it when it doesn't make sense to.

vsemozhetbyt · 2017-04-28T09:13:15Z

test/parallel/test-event-emitter-add-listeners.js

-  const listen1 = function listen1() {};
-  const listen2 = function listen2() {};
+  const listen1 = () => {};
+  const listen2 = () => {};


common.noop?

vsemozhetbyt · 2017-04-28T09:13:33Z

test/parallel/test-util-inspect.js

@@ -296,7 +296,7 @@ assert.strictEqual(

 // Function with properties
 {
-  const value = function() {};
+  const value = () => {};


common.noop?

This is another place where common.noop usage can lead to subtle bugs. The next line adds a property to the object, which would end up adding the property to common.noop which means possible side effects in other tests.

vsemozhetbyt · 2017-04-28T09:16:31Z

test/parallel/test-os.js

@@ -125,7 +125,7 @@ console.error(interfaces);
 switch (platform) {
  case 'linux':
    {
-      const filter = function(e) { return e.address === '127.0.0.1'; };
+      const filter = (e) => { return e.address === '127.0.0.1'; };


e => e.address === '127.0.0.1'?

Sure! It's the predominant style and if it inches us closer to being able to enable arrow-body-style in .eslintrc.yaml, then I'm all for it.

vsemozhetbyt · 2017-04-28T09:17:23Z

test/parallel/test-os.js

@@ -135,7 +135,7 @@ switch (platform) {
    }
  case 'win32':
    {
-      const filter = function(e) { return e.address === '127.0.0.1'; };
+      const filter = (e) => { return e.address === '127.0.0.1'; };


e => e.address === '127.0.0.1'?

e => e.address === '127.0.0.1'?

👍

vsemozhetbyt · 2017-04-28T09:19:43Z

test/parallel/test-preload.js

  let option = '';
  preloads.forEach(function(preload, index) {
    option += '-r ' + preload + ' ';
  });
  return option;
 };

-const fixture = function(name) {
+const fixture = (name) => {


const fixture = name => path.join(common.fixturesDir, name);

?

vsemozhetbyt · 2017-04-28T09:21:25Z

test/parallel/test-readline-interface.js

@@ -288,7 +288,7 @@ function isWarned(emitter) {

  // \t does not become part of the input when there is a completer function
  fi = new FakeInput();
-  const completer = function(line) {
+  const completer = (line) => {


const completer = line => [[], line];

?

vsemozhetbyt · 2017-04-28T09:23:47Z

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

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations.

Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations).

Trott · 2017-04-28T20:30:41Z

Arrow function and quotation mark nits applied.

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

Make `common.noop` a getter that returns a new function object each time the propery is read, not the same one. The old behavior could possibly lead to subtle and hard to catch bugs in some cases. Refs: nodejs#12711 (comment)

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations. PR-URL: nodejs#12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations). PR-URL: nodejs#12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Trott · 2017-05-01T22:58:24Z

Landed in a180259 and aea7269

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations. PR-URL: nodejs#12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations). PR-URL: nodejs#12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

gibfahn · 2017-06-18T22:56:15Z

@Trott would you be willing to backport this to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations. PR-URL: nodejs#12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations). PR-URL: nodejs#12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Trott · 2017-06-19T02:17:28Z

@gibfahn Backported in #13774

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations. PR-URL: #12711 Backport-PR-URL: #13774 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations). PR-URL: #12711 Backport-PR-URL: #13774 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations. PR-URL: #12711 Backport-PR-URL: #13774 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations). PR-URL: #12711 Backport-PR-URL: #13774 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations. PR-URL: nodejs#12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations. Backport-PR-URL: #19447 PR-URL: #12711 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Trott added doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Apr 28, 2017

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem. tools Issues and PRs related to the tools directory. util Issues and PRs related to the built-in util module. labels Apr 28, 2017

hiroppy reviewed Apr 28, 2017

View reviewed changes

vsemozhetbyt reviewed Apr 28, 2017

View reviewed changes

vsemozhetbyt approved these changes Apr 28, 2017

View reviewed changes

gibfahn approved these changes Apr 28, 2017

View reviewed changes

Trott force-pushed the func-style branch 2 times, most recently from f79ef5b to fbac2cf Compare April 28, 2017 20:16

Trott added 2 commits April 28, 2017 13:26

test,lib,doc: use function declarations

fe3a22a

Replace function expressions with function declarations in preparation for a lint rule requiring function declarations.

tools: require function declarations

791a56f

Except for arrow functions, require function declarations instead of function expressions via linting. This is the predominant style in our code base (77 instances of expressions to 2344 instances of declarations).

Trott force-pushed the func-style branch from fbac2cf to 791a56f Compare April 28, 2017 20:26

aqrln mentioned this pull request Apr 28, 2017

test: make common.noop a getter #12732

Closed

4 tasks

refack mentioned this pull request Apr 30, 2017

readline: refactor readline module #12755

Closed

3 tasks

Trott closed this May 1, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 18, 2017

Trott mentioned this pull request Jun 19, 2017

(v6.x backport) tools: require function declarations #13774

Closed

gibfahn added backported-to-v6.x and removed backport-requested-v6.x labels Jun 20, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Trott deleted the func-style branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: require function declarations #12711

tools: require function declarations #12711

Trott commented Apr 28, 2017

hiroppy Apr 28, 2017

mscdex Apr 28, 2017

hiroppy Apr 28, 2017

vsemozhetbyt Apr 28, 2017

Trott Apr 28, 2017

vsemozhetbyt Apr 28, 2017

Trott Apr 28, 2017

vsemozhetbyt Apr 28, 2017

Trott Apr 28, 2017 •

edited

aqrln Apr 28, 2017

Trott Apr 28, 2017

aqrln Apr 28, 2017

jasnell Apr 29, 2017

vsemozhetbyt Apr 28, 2017

vsemozhetbyt Apr 28, 2017

Trott Apr 28, 2017

vsemozhetbyt Apr 28, 2017

Trott Apr 28, 2017

vsemozhetbyt Apr 28, 2017

Trott Apr 28, 2017

vsemozhetbyt Apr 28, 2017

vsemozhetbyt Apr 28, 2017

vsemozhetbyt commented Apr 28, 2017

Trott commented Apr 28, 2017

Trott commented May 1, 2017

gibfahn commented Jun 18, 2017

Trott commented Jun 19, 2017

		@@ -9,5 +9,4 @@ test_async.Test(5, common.mustCall(function(err, val) {
		process.nextTick(common.mustCall(function() {}));

tools: require function declarations #12711

tools: require function declarations #12711

Conversation

Trott commented Apr 28, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Apr 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 28, 2017

Trott commented Apr 28, 2017

Trott commented May 1, 2017

gibfahn commented Jun 18, 2017

Trott commented Jun 19, 2017

Trott Apr 28, 2017 •

edited