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

fix: make arity error message actionable #1901

Merged
merged 1 commit into from Apr 28, 2022
Merged
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
23 changes: 21 additions & 2 deletions lib/chain.js
Expand Up @@ -73,12 +73,31 @@ Chain.prototype.getHandlers = function getHandlers() {
*/
Chain.prototype.add = function add(handler) {
assert.func(handler);
var handlerId = handler._identifier || handler._name || handler.name;
if (handler.length <= 2) {
// arity <= 2, must be AsyncFunction
assert.equal(handler.constructor.name, 'AsyncFunction');
assert.equal(
handler.constructor.name,
'AsyncFunction',
`Handler [${handlerId}] is missing a third argument (the ` +
'"next" callback) but is not an async function. Middlware ' +
'handlers can be either async/await or callback-based.' +
'Callback-based (non-async) handlers should accept three ' +
'arguments: (req, res, next). Async handler functions should ' +
'accept maximum of 2 arguments: (req, res).'
);
} else {
// otherwise shouldn't be AsyncFunction
assert.notEqual(handler.constructor.name, 'AsyncFunction');
assert.notEqual(
handler.constructor.name,
'AsyncFunction',
`Handler [${handlerId}] accepts a third argument (the 'next" ` +
'callback) but is also an async function. Middlware handlers ' +
'can be either async/await or callback-based. Async handler ' +
'functions should accept maximum of 2 arguments: (req, res). ' +
'Non-async handlers should accept three arguments: (req, ' +
'res, next).'
);
}

// _name is assigned in the server and router
Expand Down
2 changes: 2 additions & 0 deletions lib/router.js
Expand Up @@ -205,6 +205,8 @@ Router.prototype.mount = function mount(opts, handlers) {
handler._name =
handler.name || 'handler-' + self._anonymousHandlerCounter++;

handler._identifier = `${handler._name} on ${opts.method} ${opts.path}`;

// Attach to middleware chain
chain.add(handler);
});
Expand Down
21 changes: 11 additions & 10 deletions test/chain.test.js
Expand Up @@ -409,23 +409,24 @@ test('abort with throw inside async function', function(t) {
});

test('fails to add non async function with arity 2', function(t) {
var handler = function getLunch(req, res) {
res.send('ok');
};
var chain = new Chain();

t.throws(function() {
chain.add(function(req, res) {
res.send('ok');
});
}, Error);
chain.add(handler);
}, /getLunch/);
t.end();
});

test('fails to add async function with arity 3', function(t) {
var chain = new Chain();
var handler = async function getBreakfast(req, res, next) {
res.send('ok');
};

var chain = new Chain();
t.throws(function() {
chain.add(async function(req, res, next) {
res.send('ok');
});
}, Error);
chain.add(handler);
}, /getBreakfast/);
t.end();
});