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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ rules:
eol-last: 2
func-call-spacing: 2
func-name-matching: 2
func-style: [2, declaration, {allowArrowFunctions: true}]
indent: [2, 2, {ArrayExpression: first,
CallExpression: {arguments: first},
MemberExpression: 1,
Expand Down
2 changes: 1 addition & 1 deletion doc/api/util.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 😓


util.isFunction({});
// Returns: false
Expand Down
4 changes: 2 additions & 2 deletions lib/_tls_legacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -448,9 +448,9 @@ CryptoStream.prototype.destroySoon = function(err) {
// was written on this side was read from the other side.
var self = this;
var waiting = 1;
var finish = function() {
function finish() {
if (--waiting === 0) self.destroy();
};
}
this._opposite.once('end', finish);
if (!this._finished) {
this.once('finish', finish);
Expand Down
4 changes: 2 additions & 2 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -645,11 +645,11 @@ function pbkdf2(password, salt, iterations, keylen, digest, callback) {
// at this point, we need to handle encodings.
var encoding = exports.DEFAULT_ENCODING;
if (callback) {
var next = function(er, ret) {
function next(er, ret) {
if (ret)
ret = ret.toString(encoding);
callback(er, ret);
};
}
binding.PBKDF2(password, salt, iterations, keylen, digest, next);
} else {
var ret = binding.PBKDF2(password, salt, iterations, keylen, digest);
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ function cachedResult(fn) {
// B() instanceof A // true
// B() instanceof B // true
function createClassWrapper(type) {
const fn = function(...args) {
function fn(...args) {
return Reflect.construct(type, args, new.target || type);
};
}
// Mask the wrapper function name and length values
Object.defineProperties(fn, {
name: {value: type.name},
Expand Down
5 changes: 2 additions & 3 deletions test/addons-napi/test_async/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ const test_async = require(`./build/${common.buildType}/test_async`);
test_async.Test(5, common.mustCall(function(err, val) {
assert.strictEqual(err, null);
assert.strictEqual(val, 10);
process.nextTick(common.mustCall(function() {}));
process.nextTick(common.mustCall());
}));

const cancelSuceeded = function() {};
test_async.TestCancel(common.mustCall(cancelSuceeded));
test_async.TestCancel(common.mustCall());
6 changes: 3 additions & 3 deletions test/addons-napi/test_exception/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ const common = require('../../common');
const test_exception = require(`./build/${common.buildType}/test_exception`);
const assert = require('assert');
const theError = new Error('Some error');
const throwTheError = function() {
function throwTheError() {
throw theError;
};
}
let caughtError;

const throwNoError = function() {};
const throwNoError = common.noop;

// Test that the native side successfully captures the exception
let returnedError = test_exception.returnException(throwTheError);
Expand Down
4 changes: 2 additions & 2 deletions test/addons-napi/test_instanceof/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ if (typeof Symbol !== 'undefined' && 'hasInstance' in Symbol &&
(theObject instanceof theConstructor));
}

const MyClass = function MyClass() {};
function MyClass() {}
Object.defineProperty(MyClass, Symbol.hasInstance, {
value: function(candidate) {
return 'mark' in candidate;
}
});

const MySubClass = function MySubClass() {};
function MySubClass() {}
MySubClass.prototype = new MyClass();

let x = new MySubClass();
Expand Down
4 changes: 2 additions & 2 deletions test/addons/make-callback/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ const forward = vm.runInNewContext(`
})
`);
// Runs in outer context.
const endpoint = function($Object) {
function endpoint($Object) {
if (Object === $Object)
throw new Error('bad');
return Object;
};
}
assert.strictEqual(Object, makeCallback(process, forward, endpoint));
8 changes: 4 additions & 4 deletions test/inspector/inspector-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ function timeout(message, multiplicator) {
TIMEOUT * (multiplicator || 1));
}

const TestSession = function(socket, harness) {
function TestSession(socket, harness) {
this.mainScriptPath = harness.mainScriptPath;
this.mainScriptId = null;

Expand All @@ -147,7 +147,7 @@ const TestSession = function(socket, harness) {
buffer = buffer.slice(consumed);
} while (consumed);
}).on('close', () => assert(this.expectClose_, 'Socket closed prematurely'));
};
}

TestSession.prototype.scriptUrlForId = function(id) {
return this.scripts_[id];
Expand Down Expand Up @@ -304,7 +304,7 @@ TestSession.prototype.testHttpResponse = function(path, check) {
};


const Harness = function(port, childProcess) {
function Harness(port, childProcess) {
this.port = port;
this.mainScriptPath = mainScript;
this.stderrFilters_ = [];
Expand All @@ -329,7 +329,7 @@ const Harness = function(port, childProcess) {
this.returnCode_ = code;
this.running_ = false;
});
};
}

Harness.prototype.addStderrFilter = function(regexp, callback) {
this.stderrFilters_.push((message) => {
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ a.throws(makeBlock(thrower, TypeError), function(err) {

AnotherErrorType = class extends Error {};

const functionThatThrows = function() {
const functionThatThrows = () => {
throw new AnotherErrorType('foo');
};

Expand Down Expand Up @@ -493,6 +493,7 @@ a.throws(makeBlock(a.deepEqual, args, []));

// more checking that arguments objects are handled correctly
{
// eslint-disable-next-line func-style
const returnArguments = function() { return arguments; };

const someArgs = returnArguments('a');
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-child-process-fork-dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ if (process.argv[2] === 'child') {
});
});

const sendMessages = function() {
function sendMessages() {
const serverPort = parentServer.address().port;

const timer = setInterval(function() {
Expand All @@ -102,7 +102,7 @@ if (process.argv[2] === 'child') {
);
}
}, 1);
};
}

parentServer.bind(0, '127.0.0.1');

Expand Down
12 changes: 6 additions & 6 deletions test/parallel/test-child-process-fork-net.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ if (process.argv[2] === 'child') {
}));

// send net.Server to child and test by connecting
const testServer = function(callback) {
function testServer(callback) {

// destroy server execute callback when done
const progress = new ProgressTracker(2, function() {
Expand Down Expand Up @@ -116,7 +116,7 @@ if (process.argv[2] === 'child') {
server.listen(0);

// handle client messages
const messageHandlers = function(msg) {
function messageHandlers(msg) {

if (msg.what === 'listening') {
// make connections
Expand All @@ -138,13 +138,13 @@ if (process.argv[2] === 'child') {
child.removeListener('message', messageHandlers);
callback();
}
};
}

child.on('message', messageHandlers);
};
}

// send net.Socket to child
const testSocket = function(callback) {
function testSocket(callback) {

// create a new server and connect to it,
// but the socket will be handled by the child
Expand Down Expand Up @@ -179,7 +179,7 @@ if (process.argv[2] === 'child') {
server.close();
});
});
};
}

// create server and send it to child
let serverSuccess = false;
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-child-process-fork-net2.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ if (process.argv[2] === 'child') {

server.listen(0, '127.0.0.1');

const closeServer = function() {
function closeServer() {
server.close();

setTimeout(function() {
Expand All @@ -153,7 +153,7 @@ if (process.argv[2] === 'child') {
child2.send('close');
child3.disconnect();
}, 200);
};
}

process.on('exit', function() {
assert.strictEqual(disconnected, count);
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-spawn-typeerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ assert.throws(function() {
// Argument types for combinatorics
const a = [];
const o = {};
const c = function c() {};
function c() {}
const s = 'string';
const u = undefined;
const n = null;
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-cluster-disconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ if (cluster.isWorker) {
const servers = 2;

// test a single TCP server
const testConnection = function(port, cb) {
const testConnection = (port, cb) => {
const socket = net.connect(port, '127.0.0.1', () => {
// buffer result
let result = '';
Expand All @@ -52,7 +52,7 @@ if (cluster.isWorker) {
};

// test both servers created in the cluster
const testCluster = function(cb) {
const testCluster = (cb) => {
let done = 0;

for (let i = 0; i < servers; i++) {
Expand All @@ -67,7 +67,7 @@ if (cluster.isWorker) {
};

// start two workers and execute callback when both is listening
const startCluster = function(cb) {
const startCluster = (cb) => {
const workers = 8;
let online = 0;

Expand All @@ -81,7 +81,7 @@ if (cluster.isWorker) {
}
};

const test = function(again) {
const test = (again) => {
//1. start cluster
startCluster(common.mustCall(() => {
//2. test cluster
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-cluster-master-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ if (cluster.isWorker) {
// Check that the cluster died accidentally (non-zero exit code)
masterExited = !!code;

const pollWorkers = function() {
const pollWorkers = () => {
// When master is dead all workers should be dead too
let alive = false;
workers.forEach((pid) => alive = common.isAlive(pid));
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-cluster-master-kill.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ if (cluster.isWorker) {
assert.strictEqual(code, 0);

// check worker process status
const pollWorker = function() {
const pollWorker = () => {
alive = common.isAlive(pid);
if (alive) {
setTimeout(pollWorker, 50);
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-cluster-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ if (cluster.isWorker) {


let client;
const check = function(type, result) {
const check = (type, result) => {
checks[type].receive = true;
checks[type].correct = result;
console.error('check', checks);
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-cluster-worker-exit.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ if (cluster.isWorker) {
}
}));

const finish_test = function() {
const finish_test = () => {
try {
checkResults(expected_results, results);
} catch (exc) {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-console-not-call-toString.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
require('../common');
const assert = require('assert');

const func = function() {};
function func() {}
let toStringCalled = false;
func.toString = function() {
toStringCalled = true;
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-event-emitter-add-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 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?

const ee = new EventEmitter();

ee.once('newListener', function() {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-event-emitter-once.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ e.emit('hello', 'a', 'b');
e.emit('hello', 'a', 'b');
e.emit('hello', 'a', 'b');

const remove = function() {
function remove() {
assert.fail('once->foo should not be emitted');
};
}

e.once('foo', remove);
e.removeListener('foo', remove);
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-access.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ const doesNotExist = path.join(common.tmpDir, '__this_should_not_exist');
const readOnlyFile = path.join(common.tmpDir, 'read_only_file');
const readWriteFile = path.join(common.tmpDir, 'read_write_file');

const createFileWithPerms = function(file, mode) {
function createFileWithPerms(file, mode) {
fs.writeFileSync(file, '');
fs.chmodSync(file, mode);
};
}

common.refreshTmpDir();
createFileWithPerms(readOnlyFile, 0o444);
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-link.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ const srcPath = path.join(common.tmpDir, 'hardlink-target.txt');
const dstPath = path.join(common.tmpDir, 'link1.js');
fs.writeFileSync(srcPath, 'hello world');

const callback = function(err) {
function callback(err) {
assert.ifError(err);
const dstContent = fs.readFileSync(dstPath, 'utf8');
assert.strictEqual('hello world', dstContent);
};
}

fs.link(srcPath, dstPath, common.mustCall(callback));

Expand Down