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

Add promise support #25

Closed
wants to merge 12 commits into from
45 changes: 29 additions & 16 deletions lib/layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,31 @@ 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 next = arguments[arguments.length - 1]
var handle = this.handle
var result
var hasNextBeenCalled = function () { return next._currentLayer === self }
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than assigning an anonymous function to a variable, just change this to a named function declaration. This means that runtime will have one less variable initialization + assignment and also an error occurring within the function for whatever reason will have the function's name in the stack trace instead of it being anonymous.

Copy link
Author

Choose a reason for hiding this comment

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

Best case scenario the fix in #26 means we don't need this. But I'll definitely fix if we do.


try {
result = handle.apply(undefined, arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

Prior to this change, the handle was called with the default context (global in sloppy mode and null in strict mode). Now this is being changed to always provide a context of undefined. This will probably break some middleware. Was there a reason to change the context of the handle call? Can we search npm to determine the swath of modules this change would effect?

Copy link
Member

Choose a reason for hiding this comment

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

@dougwilson If I understand, this will still function the same as this is already under strict mode. Also, call and apply actually work the same here (when you pass undefined in non-strict mode it would be global, but this file is strict).

Copy link
Author

Choose a reason for hiding this comment

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

I did some basic research and undefined appeared to be the most canonical thing to use. How would we fix this in the scenario where it might be useful to give some people the global object?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi guys, what determines the context in this case is the strict mode of the handle function, which is determined by the calling code outside of this module; the use of strict in this source code does not affect the context that handle ultimately ends up with.

You can see this displayed right here:

$ node -e 'var a=(function(){"use strict";return function(){console.log(String(this))}}()),b=(function(){return function(){console.log(String(this))}}());(function(){"use strict";console.log("strict caller");console.log(String(this));console.log("strict callee");a();console.log("sloppy callee");b()}())'
strict caller
undefined
strict callee
undefined
sloppy callee
[object global]

This right now is definitely a breaking point of this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, changed undefined to (function () { return this })() which works in theory. Here is my test:

function strict() {
  'use strict';
  context.fn();
}

function sloppy() {
  context.fn();
}

var context = {
  a: 1,
  fn: function () {
    console.log(this.a);
    console.log((function () { return !!this })());
  }
};

strict();
sloppy();

and output:

1
true
1
true

Although I'm getting a different result from you (as you can see), both are returning the global object implying the 'use strict'; in strict() does nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @calebmer , your change does not fix it, because the this in your code is still executing from within the strict code in the router source code, which is not where the this varies at.

Your test above is also very different from the test I gave, as the this is always being executed in sloppy mode, not in the caller's defined code. Please refer to my example for what I'm talking about :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, the this is defined by where the physical this is located, it will not vary depending on the call stack, which is what the issue is. In your test, the this for both calls is physically located in the same spot, this why it always evaluates to the same thing. My example demonstrates that the this is going to be physically located in other people's code, not within the router source code. Your use of .apply overrides this behavior, which is likely to result in unexpected behavior (Node.js code experienced this a lot when they tried to change the context for setTimeout execution).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so I created a test for this, so really, all that would matter if the following test passed (ensure nowhere else in the file with the tests there is a 'use strict'):

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)
    })
  })
})

Copy link
Author

Choose a reason for hiding this comment

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

Added the tests verbatim and they worked with both (function () { return this })() and undefined interestingly enough. So I switched back to undefined. Want to take a look?

} catch (err) {
return next(err)
}

if (result != null && typeof result.then === 'function') {
var onRejected = function (error) { if (!hasNextBeenCalled()) { next(error) } }
Copy link
Member

Choose a reason for hiding this comment

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

Just a note, but we should probably use next(error || new Error('something here') as promises can be rejected with falsy values.

Copy link
Author

Choose a reason for hiding this comment

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

Added an error, but maybe someone would want to reject(false) and expect this behavior? I would not understand why so I agree with you.

result.then(null, onRejected)
}
}

/**
* Handle the error for the layer.
*
Expand All @@ -58,18 +83,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)
}

/**
Expand All @@ -82,18 +101,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)
}

/**
Expand Down
3 changes: 3 additions & 0 deletions lib/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
88 changes: 88 additions & 0 deletions test/promise.js
Original file line number Diff line number Diff line change
@@ -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++
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than asserting counts with a bunch of closures, please use the x-hit stuff from the test suite and assert against the response headers.

Copy link
Author

Choose a reason for hiding this comment

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

The count method I have found especially useful at places like L36 where I can test a closure has not been called. Is there a way to do this with the x-hit stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, simply call done(new Error('unexpected call')) in the thing that should be called instead of trying to do the counting (because it could still end up getting called, but after you checked the count and then your test would incorrectly pass).

Copy link
Author

Choose a reason for hiding this comment

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

Fixed the tests, also created a createErrorHitHandle (in test/support/utils.js) middleware which also excepts the fourth error parameter. That ok with you?

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++
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than asserting counts with a bunch of closures, please use the x-hit stuff from the test suite and assert against the response headers.

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)
})
})