Skip to content

Commit

Permalink
feat: remove re-routing from handler (#1847)
Browse files Browse the repository at this point in the history 
BREAKING CHANGE: deprecates and removes re-routing when passing a string parameter to `next()`
  • Loading branch information
@ghermeto
ghermeto committed Jul 14, 2020
1 parent 71ac3a0 commit 9153587
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 228 deletions.
7 changes: 5 additions & 2 deletions lib/chain.js
View file
Expand Up @@ -169,10 +169,13 @@ function call(handler, err, req, res, _next) {
function resolve(value) {
if (value && req.log) {
// logs resolved value
req.log.debug({ value }, 'Async handler resolved with a value');
req.log.warn(
{ value },
'Discarded returned value from async handler'
);
}

return next(value);
return next();
}

function reject(error) {
Expand Down
21 changes: 0 additions & 21 deletions lib/server.js
View file
Expand Up @@ -1242,27 +1242,6 @@ Server.prototype._onHandlerError = function _onHandlerError(
) {
var self = this;

// Call route by name
if (!isUncaught && _.isString(err)) {
var routeName = err;
var routeHandler = self.router.lookupByName(routeName, req, res);

// Cannot find route by name, called when next('route-name') doesn't
// find any route, it's a 5xx error as it's a programatic error
if (!routeHandler) {
var routeByNameErr = new customErrorTypes.RouteMissingError(
"Route by name doesn't exist"
);
routeByNameErr.code = 'ENOEXIST';
self._afterRoute(routeByNameErr, req, res);
return;
}
routeHandler(req, res, function afterRouter(routeErr) {
self._afterRoute(routeErr, req, res);
});
return;
}

// Handlers don't continue when error happen
res._handlersFinished = true;

Expand Down
213 changes: 8 additions & 205 deletions test/server.test.js
View file
Expand Up @@ -884,93 +884,6 @@ test('GH #704: Route with an invalid RegExp params', function(t) {
});
});

test('gh-193 basic', function(t) {
SERVER.get(
{
name: 'foo',
path: '/foo'
},
function(req, res, next) {
next('bar');
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function(req, res, next) {
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.end();
});
});

test('gh-193 route name normalization', function(t) {
SERVER.get(
{
name: 'foo',
path: '/foo'
},
function(req, res, next) {
next('b-a-r');
}
);

SERVER.get(
{
name: 'b-a-r',
path: '/bar'
},
function(req, res, next) {
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.end();
});
});

test('gh-193 route ENOEXIST', function(t) {
SERVER.get(
{
name: 'foo',
path: '/foo'
},
function(req, res, next) {
next('baz');
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function(req, res, next) {
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ok(err);
t.equal(res.statusCode, 500);
t.end();
});
});

test('run param only with existing req.params', function(t) {
var count = 0;

Expand Down Expand Up @@ -1023,91 +936,7 @@ test('run param only with existing req.params', function(t) {
});
});

test('gh-193 route only run use once', function(t) {
var count = 0;

SERVER.use(function(req, res, next) {
count++;
next();
});

SERVER.get(
{
name: 'foo',
path: '/foo'
},
function(req, res, next) {
next('bar');
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function(req, res, next) {
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(count, 1);
t.end();
});
});

test('gh-193 route chained', function(t) {
var count = 0;

SERVER.use(function addCounter(req, res, next) {
count++;
next();
});

SERVER.get(
{
name: 'foo',
path: '/foo'
},
function getFoo(req, res, next) {
next('bar');
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function getBar(req, res, next) {
next('baz');
}
);

SERVER.get(
{
name: 'baz',
path: '/baz'
},
function getBaz(req, res, next) {
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(count, 1);
t.end();
});
});

test('gh-193 route params basic', function(t) {
test('next("string") returns InternalServer', function(t) {
var count = 0;

SERVER.use(function(req, res, next) {
Expand All @@ -1126,27 +955,15 @@ test('gh-193 route params basic', function(t) {
}
);

SERVER.get(
{
name: 'bar',
path: '/bar/:baz'
},
function(req, res, next) {
t.notOk(req.params.baz);
res.send(200);
next();
}
);

CLIENT.get('/foo/blah', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.ok(err);
t.equal(res.statusCode, 500);
t.equal(count, 1);
t.end();
});
});

test('gh-193 next("route") from a use plugin', function(t) {
test('next("string") from a use plugin returns InternalServer', function(t) {
var count = 0;

SERVER.use(function plugin(req, res, next) {
Expand All @@ -1160,25 +977,14 @@ test('gh-193 next("route") from a use plugin', function(t) {
path: '/foo'
},
function getFoo(req, res, next) {
res.send(500);
next();
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function getBar(req, res, next) {
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.ok(err);
t.equal(res.statusCode, 500);
t.equal(count, 1);
t.end();
});
Expand Down Expand Up @@ -3000,14 +2806,11 @@ test('async handler without next', function(t) {
});
});

test('async handler resolved with string should re-route', function(t) {
test('async handler should discard value', function(t) {
SERVER.get('/hello/:name', async function tester(req, res) {
await helper.sleep(10);
return 'getredirected';
});

SERVER.get('/redirected', async function tester(req, res) {
res.send(req.params.name);
return 'foo';
});

CLIENT.get('/hello/mark', function(err, _, res) {
Expand Down

0 comments on commit 9153587

Please sign in to comment.