Add promise support

[calebmer]

Moved from expressjs/express#2763

Once this is ready to merge, I want to try to implement the next plan from expressjs/express#2259, which would require an optional Promise library.

Also, I'm not familiar with your policy for backwards compatibility (or if one exists at all) in express 5.0, that could be helpful to know.

[dougwilson]

Also, I'm not familiar with your policy for backwards compatibility (or if one exists at all) in express 5.0, that could be helpful to know.

It depends on what is not backwards-compatible. If we are saying that suddenly all existing middleware will no longer function, that is a really big thing and would require a really long time to determine if that is desired, get the community to update, etc. Breakage of any current middleware pattern can be done, but it just has to have a big enough benefit to justify community-wide breakage and pain. I hope that provides some kind of benchmark :)

[blakeembrey]

Don't pop and concat later, you could just use arguments[arguments.length - 1] and apply using arguments.

[calebmer]

Yeah that's a vestigal structure, fixed 😄

[blakeembrey]

This would be a really cool addition 😄 The only thing that probably needs some discussion is the onRejected, but it looks like this will be cool. The upstream handling will be amazing for a future PR.

Edit: About onRejected, I don't know if this should continue with the same behavior as existing and just call next twice. To get in that state is a bigger issue overall and something not handled currently, so it might be better to handle in one swoop and throw an error when calling next twice instead of failing silently.

[blakeembrey]

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

[calebmer]

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.

[calebmer]

My reasoning for onRejected is there is an automatic try/catch block for sync errors, so it makes sense to have an automatic catch for async errors.

[blakeembrey]

@calebmer Sorry, onRejected is fine, I was specifically talking about the hasNextBeenCalled handling.

Edit: See my previous edit. Sorry 😝

[calebmer]

Yes, that is true. Do we agree that hasNextBeenCalled (or some derivative) is necessary?

[blakeembrey]

It's up to @dougwilson here, but I don't think it belongs in this PR. We should make a new issue that addresses multiple next calls.

[dougwilson]

Yes, please keep a PR to only making one change at a time so discussions are easy to follow and we can accept them on independent timelines. Addressing multiple next() calls should be a different PR.

[calebmer]

The practical solution for this PR is here. Would you like me to move this into its own PR?

[blakeembrey]

@calebmer Yes, don't try to do it as part of this PR. It would also need to work for the normal callbacks multiple times.

[dougwilson]

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?

[blakeembrey]

@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).

[calebmer]

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?

[dougwilson]

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.

[calebmer]

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.

[dougwilson]

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

[dougwilson]

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

[dougwilson]

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

[calebmer]

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?

[calebmer]

I am going to close this for now until #26 is accepted and the new branch is created. This is so I can do development on the master branch of my fork for npm installs.

[blakeembrey]

@calebmer How were these two things tied together? I would still love to see promise support in the router.

[calebmer]

@blakeembrey in async/await functions a developer might call next then throw an error afterwards calling next again.

Along with a couple other weird async multiple next call scenarios, it would be helpful to emit an error/prevent it from happening. That's the goal of #26 and the hasNextBeenCalled appriach earlier.

Mainly I'm closing (not forever 😊) so I can continue development on my master, specifically implementing some ideas from expressjs/express#2259 (koa style middleware)

[blakeembrey]

@calebmer Fair enough. I experimented with a promise attempt a while back to support upstream calls, but it was a bit of a mess to stay backward compatible. It'd be good to have it working for router@2. The main thing I ended up doing was adding next.resolve and next.reject for simpler chaining, IIRC I have a perf degradation with the way I was trying to solve it too.

[calebmer]

@blakeembrey if you want to follow future development while we wait for #26 to get merged it lives at calebmer/router. next() returning a promise could be helpful for an app I am developing so I will be trying out the changes which are documented in the readme.

[frogcjn]

Any progress? @calebmer, @blakeembrey

[calebmer]

I wish, but it's out of my hands atm 😖

I'm not sure who has the power to do Express releases at this point, maybe the Node.js TSC? You'll need to dig up who has the authority to make this call and bug them.