From cc435349d126e3fc54b1ec00259dea0c0b012d82 Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 19:31:59 -0400 Subject: [PATCH 01/11] Add promise support --- lib/layer.js | 47 +++++++++++++++++--------- lib/route.js | 3 ++ package.json | 1 + test/promise.js | 88 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 16 deletions(-) create mode 100644 test/promise.js diff --git a/lib/layer.js b/lib/layer.js index 99b25d2..0beff65 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -47,6 +47,33 @@ function Layer(path, options, fn) { } } +/** + * Handle any request for the layer. + * + * @api private + */ + +Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { + var self = this + var args = Array.prototype.slice.call(arguments) + var next = args.pop() + var handle = this.handle + var result + var hasNextBeenCalled = function () { return next._currentLayer === self } + + try { + result = handle.apply(undefined, args.concat([next])) + } catch (err) { + return next(err) + } + + if (result != null && typeof result.then === 'function') { + var onFulfilled = function () { /* Silence... */ } + var onRejected = function (error) { if (!hasNextBeenCalled()) { next(error) } } + result.then(onFulfilled, onRejected) + } +} + /** * Handle the error for the layer. * @@ -58,18 +85,12 @@ function Layer(path, options, fn) { */ Layer.prototype.handle_error = function handle_error(error, req, res, next) { - var fn = this.handle - - if (fn.length !== 4) { + if (this.handle.length !== 4) { // not a standard error handler return next(error) } - try { - fn(error, req, res, next) - } catch (err) { - next(err) - } + this._handle_any(error, req, res, next) } /** @@ -82,18 +103,12 @@ Layer.prototype.handle_error = function handle_error(error, req, res, next) { */ Layer.prototype.handle_request = function handle(req, res, next) { - var fn = this.handle - - if (fn.length > 3) { + if (this.handle.length > 3) { // not a standard request handler return next() } - try { - fn(req, res, next) - } catch (err) { - next(err) - } + this._handle_any(req, res, next) } /** diff --git a/lib/route.js b/lib/route.js index 8b67f2d..1a954a6 100644 --- a/lib/route.js +++ b/lib/route.js @@ -127,6 +127,9 @@ Route.prototype.dispatch = function dispatch(req, res, done) { match = !layer.method || layer.method === method } + // set a reference to the current layer + next._currentLayer = layer + // no match if (match !== true) { return done(err) diff --git a/package.json b/package.json index de73033..4122b91 100644 --- a/package.json +++ b/package.json @@ -16,6 +16,7 @@ }, "devDependencies": { "after": "0.8.1", + "bluebird": "^2.10.2", "finalhandler": "0.4.0", "istanbul": "0.3.17", "mocha": "2.2.5", diff --git a/test/promise.js b/test/promise.js new file mode 100644 index 0000000..8cc1b0a --- /dev/null +++ b/test/promise.js @@ -0,0 +1,88 @@ + +var Router = require('..') +var utils = require('./support/utils') +var Promise = require('bluebird') + +var assert = utils.assert +var createServer = utils.createServer +var request = utils.request + +describe('Promise', function () { + it('rejecting will trigger error handlers', function (done) { + var router = Router() + var server = createServer(router) + + router.use(function (req, res) { + return new Promise(function (resolve, reject) { + reject(new Error('Happy error')) + }) + }) + + request(server) + .get('/') + .expect(500, done) + }) + + it('will be ignored if next is called', function (done) { + var router = Router() + var server = createServer(router) + var count = 0 + + router.use(function (req, res, next) { + count++ + return new Promise(function (resolve, reject) { + count++ + next() + setTimeout(function () { + count++ + resolve() + }, 5) + }) + }) + + router.use(function (req, res) { + assert.equal(count, 2) + res.end('Awesome!') + }) + + request(server) + .get('/') + .expect(200, done) + }) + + it('can be used in error handlers', function (done) { + var router = Router() + var server = createServer(router) + var count = 0 + + router.use(function (req, res, next) { + count++ + next(new Error('Happy error')) + }) + + router.use(function (error, req, res, next) { + count++ + return new Promise(function (resolve, reject) { + count++ + setTimeout(function () { + count++ + next(error) + resolve() + }, 5) + }) + }) + + router.use(function () { + done(new Error('This should never be reached')) + }) + + router.use(function (error, req, res, next) { + assert.equal(count, 4) + res.end('Awesome!') + }) + + request(server) + .get('/') + .expect(200, done) + }) +}) From f5a61453c89a55e49d7e5b4771affa1eabacb465 Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 19:41:49 -0400 Subject: [PATCH 02/11] Provide explicit bluebird version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 4122b91..6976d05 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ }, "devDependencies": { "after": "0.8.1", - "bluebird": "^2.10.2", + "bluebird": "2.10.2", "finalhandler": "0.4.0", "istanbul": "0.3.17", "mocha": "2.2.5", From 3e5be32f5254b283096df969e4dba5c25411480b Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 20:05:32 -0400 Subject: [PATCH 03/11] Remove pop/concat --- lib/layer.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/layer.js b/lib/layer.js index 0beff65..2ccb6e6 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -55,14 +55,13 @@ function Layer(path, options, fn) { Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { var self = this - var args = Array.prototype.slice.call(arguments) - var next = args.pop() + var next = arguments[arguments.length - 1] var handle = this.handle var result var hasNextBeenCalled = function () { return next._currentLayer === self } try { - result = handle.apply(undefined, args.concat([next])) + result = handle.apply(undefined, arguments) } catch (err) { return next(err) } From 3a925d6e3a49ee42bb42c639300cf0373ecf0a45 Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 20:06:11 -0400 Subject: [PATCH 04/11] Remove onFullfiled callback --- lib/layer.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/layer.js b/lib/layer.js index 2ccb6e6..7204a7d 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -67,9 +67,8 @@ Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { } if (result != null && typeof result.then === 'function') { - var onFulfilled = function () { /* Silence... */ } var onRejected = function (error) { if (!hasNextBeenCalled()) { next(error) } } - result.then(onFulfilled, onRejected) + result.then(null, onRejected) } } From 3a21cd384caf8ae1445e6c6ea5219dbf7644b1c0 Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 20:10:13 -0400 Subject: [PATCH 05/11] Add error for falsey values --- lib/layer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/layer.js b/lib/layer.js index 7204a7d..3c268fb 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -67,7 +67,7 @@ Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { } if (result != null && typeof result.then === 'function') { - var onRejected = function (error) { if (!hasNextBeenCalled()) { next(error) } } + var onRejected = function (error) { if (!hasNextBeenCalled()) { next(error || new Error('Promise rejected with falsey value')) } } result.then(null, onRejected) } } From 07a227f59171a81c3fa4eb47dd4462fe663506cf Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 22:02:26 -0400 Subject: [PATCH 06/11] Anonymous function naming --- lib/layer.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/layer.js b/lib/layer.js index 3c268fb..03a3dd8 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -58,7 +58,7 @@ Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { var next = arguments[arguments.length - 1] var handle = this.handle var result - var hasNextBeenCalled = function () { return next._currentLayer === self } + function hasNextBeenCalled() { return next._currentLayer === self } try { result = handle.apply(undefined, arguments) @@ -67,8 +67,10 @@ Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { } if (result != null && typeof result.then === 'function') { - var onRejected = function (error) { if (!hasNextBeenCalled()) { next(error || new Error('Promise rejected with falsey value')) } } - result.then(null, onRejected) + result.then( + null, + function onRejected(error) { if (!hasNextBeenCalled()) { next(error || new Error('Promise rejected with falsey value')) } } + ) } } From 08551f1a7eee69141719b649a80e5e20fbdee3ef Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 22:46:03 -0400 Subject: [PATCH 07/11] Fix spec inconsisten thisArg --- lib/layer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/layer.js b/lib/layer.js index 03a3dd8..ae709c8 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -61,7 +61,7 @@ Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { function hasNextBeenCalled() { return next._currentLayer === self } try { - result = handle.apply(undefined, arguments) + result = handle.apply((function () { return this })(), arguments) } catch (err) { return next(err) } From 1a6f58109854626cc61d0087a8c0fc55c46a8955 Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 22:46:03 -0400 Subject: [PATCH 08/11] Fix spec inconsistent this --- lib/layer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/layer.js b/lib/layer.js index 03a3dd8..ae709c8 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -61,7 +61,7 @@ Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { function hasNextBeenCalled() { return next._currentLayer === self } try { - result = handle.apply(undefined, arguments) + result = handle.apply((function () { return this })(), arguments) } catch (err) { return next(err) } From 1f312b3753790fc1a626dc35f794b54e64de151a Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 22:59:34 -0400 Subject: [PATCH 09/11] Better tests --- test/promise.js | 32 ++++++++++++++++---------------- test/support/utils.js | 8 ++++++++ 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/test/promise.js b/test/promise.js index 8cc1b0a..8e71929 100644 --- a/test/promise.js +++ b/test/promise.js @@ -4,6 +4,9 @@ var utils = require('./support/utils') var Promise = require('bluebird') var assert = utils.assert +var createHitHandle = utils.createHitHandle +var createErrorHitHandle = utils.createErrorHitHandle +var shouldHitHandle = utils.shouldHitHandle var createServer = utils.createServer var request = utils.request @@ -26,46 +29,41 @@ describe('Promise', function () { it('will be ignored if next is called', function (done) { var router = Router() var server = createServer(router) - var count = 0 + var timeoutCalled = false - router.use(function (req, res, next) { - count++ + router.use(createHitHandle(1), function (req, res, next) { return new Promise(function (resolve, reject) { - count++ next() setTimeout(function () { - count++ + timeoutCalled = true resolve() }, 5) }) }) - router.use(function (req, res) { - assert.equal(count, 2) + router.use(createHitHandle(2), function (req, res) { + assert(!timeoutCalled) res.end('Awesome!') }) request(server) .get('/') + .expect(shouldHitHandle(1)) + .expect(shouldHitHandle(2)) .expect(200, done) }) it('can be used in error handlers', function (done) { var router = Router() var server = createServer(router) - var count = 0 - router.use(function (req, res, next) { - count++ + router.use(createHitHandle(1), function (req, res, next) { next(new Error('Happy error')) }) - router.use(function (error, req, res, next) { - count++ + router.use(createErrorHitHandle(2), function (error, req, res, next) { return new Promise(function (resolve, reject) { - count++ setTimeout(function () { - count++ next(error) resolve() }, 5) @@ -76,13 +74,15 @@ describe('Promise', function () { done(new Error('This should never be reached')) }) - router.use(function (error, req, res, next) { - assert.equal(count, 4) + router.use(createErrorHitHandle(3), function (error, req, res, next) { res.end('Awesome!') }) request(server) .get('/') + .expect(shouldHitHandle(1)) + .expect(shouldHitHandle(2)) + .expect(shouldHitHandle(3)) .expect(200, done) }) }) diff --git a/test/support/utils.js b/test/support/utils.js index 8d115f4..a04b577 100644 --- a/test/support/utils.js +++ b/test/support/utils.js @@ -6,6 +6,7 @@ var request = require('supertest') exports.assert = assert exports.createHitHandle = createHitHandle +exports.createErrorHitHandle = createErrorHitHandle exports.createServer = createServer exports.rawrequest = rawrequest exports.request = request @@ -20,6 +21,13 @@ function createHitHandle(num) { } } +function createErrorHitHandle(num) { + var hit = createHitHandle(num) + return function hitError(error, req, res, next) { + hit(req, res, function () { next(error) }) + } +} + function createServer(router) { return http.createServer(function onRequest(req, res) { router(req, res, finalhandler(req, res)) From 79a4ee011763e5f8aa251ba354fc6635087d9097 Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 23:14:36 -0400 Subject: [PATCH 10/11] Add context verification tests --- lib/layer.js | 2 +- test/this.js | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 test/this.js diff --git a/lib/layer.js b/lib/layer.js index ae709c8..03a3dd8 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -61,7 +61,7 @@ Layer.prototype._handle_any = function _handle_any(/* ...args, next */) { function hasNextBeenCalled() { return next._currentLayer === self } try { - result = handle.apply((function () { return this })(), arguments) + result = handle.apply(undefined, arguments) } catch (err) { return next(err) } diff --git a/test/this.js b/test/this.js new file mode 100644 index 0000000..30920fe --- /dev/null +++ b/test/this.js @@ -0,0 +1,41 @@ +// IMPORTANT: Do not include 'use strict' anywhere else in this file +// @see https://github.com/pillarjs/router/pull/25#discussion_r40988513 + +var Router = require('..') +var utils = require('./support/utils') + +var createServer = utils.createServer +var request = utils.request + +describe('this', function () { + describe('when middleware is sloppy', function () { + it('should have global context', function (done) { + var router = new Router() + var server = createServer(router) + + router.get('/', function (req, res) { + res.end(String(this === global)) + }) + + request(server) + .get('/') + .expect(200, 'true', done) + }) + }) + + describe('when middleware is strict', function () { + it('should have null context', function (done) { + 'use strict' + var router = new Router() + var server = createServer(router) + + router.get('/', function (req, res) { + res.end(String(this === global)) + }) + + request(server) + .get('/') + .expect(200, 'false', done) + }) + }) +}) From a7814d4baf8c351fcaa2810d50c37db792a9aa88 Mon Sep 17 00:00:00 2001 From: calebmer Date: Thu, 1 Oct 2015 23:19:08 -0400 Subject: [PATCH 11/11] Rename test file --- test/{this.js => strict.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{this.js => strict.js} (100%) diff --git a/test/this.js b/test/strict.js similarity index 100% rename from test/this.js rename to test/strict.js