-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feat(middleware): Return a promise that resolves when middleware stack done #1339
Conversation
Just noticed Travis failed, but only on Node 0.1.0 due to lack of promise support. |
Hubot 3 will certainly have new node version requirement, but there is still many 0.10 out there (cf #1336), I think it would be bad to introduce a breaking change for them at this point. |
@timkinnane thanks for the pull request! I think we should wait until the next major release to drop support for v0.10. In the mean time, I'm not opposed to adding a dependency so we can start using promises now. es6-promise would be my preference so we can use the native Promise = global.Promise || require('es6-promise').Promise |
Builds in node 0.10.x were failing due to lack of promise support 1339
You can commit changes to |
src/middleware.coffee
Outdated
@@ -1,4 +1,5 @@ | |||
async = require 'async' | |||
Promise = global.Promise || require('es6-promise').Promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think https://github.com/stefanpenner/es6-promise#auto-polyfill means you could require the auto-polyfill, and then just use Promise
. On the other hand, using this style is a lot more clear on where Promise is coming from.
I think you'd need to get promises involved a little earlier in the Maybe if you wrap more of the method in the promise call, you'd be able to resolve/reject during those checks? |
Allows rejecting if middleware throws related hubotio#1339
Sweet, I've made those changes and added docs. I added one test that fails, because I need some advice to fix it. The rejection part worked though. |
test/middleware_test.coffee
Outdated
@@ -238,7 +296,7 @@ describe 'Middleware', -> | |||
{} | |||
middlewareFinished | |||
middlewareFailed | |||
) | |||
).catch (reason) -> # supress warning re unhandled promise rejection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this start showing unexpected warnings when people upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without that line 299, it was logging a warning in the tests, where middleware threw. Saying "unhandled promise rejection" - not a big deal, but simply catching the promise and doing nothing with it removes the warning. It would only effect people who's middleware is also throwing, in which case they'd see errors anyway.
I'm having trouble wrapping my head around this at the moment. You basically want calling |
@bkeepers yeah, so any middleware in the stack can prematurely end it, but that's not really a fail, so the promise should still resolve with the context at that point. I just can't see how to hook in the resolve method at that point. The way I've done it only works through the final done function. The test I added at line 240 demos how it should work. I'm sure I'll be able to fix this myself, just wanted throw it up in case someone has a simple solution. |
Fixed the test I added, for resolving promise early if middleware calls done() |
src/middleware.coffee
Outdated
@robot.emit('error', err, context.response) | ||
# Forcibly fail the middleware and stop executing deeper | ||
doneFunc() | ||
reject err, context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise.reject()
only accepts one argument, you can try it out using
Promise.reject(new Error('foo'), {}).catch(function (error, context) {
console.log(context)
})
// logs `undefined`
What you could do is set context as property of err
before rejecting with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one. Thanks for that info, I'll do it that way instead.
@@ -702,6 +702,10 @@ Every middleware receives the same API signature of `context`, `next`, and | |||
`done`. Different kinds of middleware may receive different information in the | |||
`context` object. For more details, see the API for each type of middleware. | |||
|
|||
Middleware execution returns a promise that resolves with the final `context` | |||
when the middleware stack completes. If the middleware stack throws an error, | |||
the promise will be rejected with the error and `context` at that point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my other comment, a promise can only reject with one argument
hey @timkinnane I gave your PR finally a more thorough look today. One thing that really confuses me is this part:
I try to wrap my head around it but don’t understand why that is. Could someone explain it to me? Is it so that a there can be an array of registered middleware which all do their own checks and if one of them finds that the request is for them, then we don’t want the next middleware to even do its check? I know your PR did not introduce this but I’m still rather new to the code base and it would help me to understand the thinking behind this pattern /cc @technicalpickles |
@gr2m, so normally it would run all middleware methods in the order they're registered, then a final function when complete, but each middleware in the queue can end the processing of the stack. The result of finishing early is that the "final" function never runs, but it isn't necessarily an error, just a different kind of conclusion. e.g. A listener middleware might prevent further middleware from processing if the user isn't authorised to access the listener, then reply with some "you are not valid" message instead of proceeding to the response callback. In such cases the promise should still resolve even though the stack was "cancelled" because I'm currently away and will be for another week, but I'll put more work into this when I get back, to help it get through with a more consistent approach if required. |
thanks for the clarifications! Note that we will release a new major version of hubot soon which is now based on JavaScript and has a few minor breaking changes. But if for some reason you can’t upgrade to it we can release another |
Cheers, I've written a lot of code for 2.x and the promise feature is most useful for me when running mocha tests of those scripts. So I'd rather have it available without needing to update those scripts, but I'll look to evolve my packages to work better with v3 over time. |
# Conflicts: # src/middleware.coffee # src/response.coffee # src/robot.coffee # test/middleware_test.coffee Migrated promise features from removed coffee into js
Made a couple new branches to resubmit this separately for v2 and v3 |
Fixes #1338
Related #1279
Needs help / feedback...