Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: require function declarations #12711

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 28, 2017

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

@Trott 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 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
@@ -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 = () => {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.noop ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex Oops, thanks 😓

.eslintrc.yaml Outdated
@@ -97,6 +97,7 @@ rules:
eol-last: 2
func-call-spacing: 2
func-name-matching: 2
func-style: [2, "declaration", {allowArrowFunctions: true}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the quotes be omitted here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the quotes be omitted here?

Yes, indeed. Done!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it also be common.mustCall()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it also be common.mustCall()?

Sure seems like it! Done! Thanks!

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

{
const listen1 = function listen1() {};
const listen2 = function listen2() {};
const listen1 = () => {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.noop?

Copy link
Member Author

@Trott Trott Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const listen1 = function listen1() {};
const listen2 = function listen2() {};
const listen1 = () => {};
const listen2 = () => {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.noop?

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

// Function with properties
{
const value = function() {};
const value = () => {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.noop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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'; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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'; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

👍

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

const fixture = function(name) {
const fixture = (name) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

?

@@ -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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

?

@vsemozhetbyt
Copy link
Contributor

@Trott Trott force-pushed the func-style branch 2 times, most recently from f79ef5b to fbac2cf Compare April 28, 2017 20:16
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
Copy link
Member Author

Trott commented Apr 28, 2017

Arrow function and quotation mark nits applied.

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

aqrln added a commit to aqrln/node that referenced this pull request Apr 28, 2017
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)
@aqrln aqrln mentioned this pull request Apr 28, 2017
4 tasks
@refack refack mentioned this pull request Apr 30, 2017
3 tasks
Trott added a commit to Trott/io.js that referenced this pull request May 1, 2017
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>
Trott added a commit to Trott/io.js that referenced this pull request May 1, 2017
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
Copy link
Member Author

Trott commented May 1, 2017

Landed in a180259 and aea7269

@Trott Trott closed this May 1, 2017
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

@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.

Trott added a commit to Trott/io.js that referenced this pull request Jun 19, 2017
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>
Trott added a commit to Trott/io.js that referenced this pull request Jun 19, 2017
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
Copy link
Member Author

Trott commented Jun 19, 2017

@gibfahn Backported in #13774

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
@Trott Trott deleted the func-style branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. 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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants