Updating library for asynchronous support through promises #171
Conversation
|
If @mreinstein would like to do a constructive code review here that'd be much appreciated. Removing non-promised versions to simplify the code in the medium term and #22 is what we're trying to accomplish. |
|
Also see #173. |
ajcrites
force-pushed
the
force-chain-134
branch
3 times, most recently
from
ad2a1e0
to
5ee51c7
Compare
index.js
Outdated
| if (e.message) { | ||
| return response.fail("Unhandled exception: " + e.message + ".", e); | ||
| } else { | ||
| return response.fail("Unhandled exception.", e); |
There was a problem hiding this comment.
e often contains a string and this should be part of the .fail() call, especially if e is an indexed message (self.messages[e]`).
There was a problem hiding this comment.
Do you mean something like response.fail("Unhandled exception: " + e, e)?
index.js
Outdated
| }) | ||
| .catch(function(e) { | ||
| if (typeof self.error == "function") { | ||
| self.error(e, request, response); |
There was a problem hiding this comment.
I think the default behavior should be to resolve by calling response.send() here as the user error handler, app.error() can still explicitly fail if desired.
There was a problem hiding this comment.
See comments on #182 -- I can change this to return self.error and update the documentation to say that error can be async.
There was a problem hiding this comment.
I think it should next expect the error handler to return anything in release v3. Instead a call to resolve.send() if it has not been already resolved should suffice. I submitted PR #183 to handle.
ajcrites
force-pushed
the
force-chain-134
branch
from
5ee51c7
to
2a839a3
Compare
|
@dblock this is now up-to-date with current changes and makes Anything else we need to do to proceed with this update? We can work off of a v4 branch and create an rc rather than using master if we want to spend some time with v3. |
|
Can you take a look at #186 and merge it if it makes sense? I think @tejashah88 is not around. Then make a 3.2.0 release (follow RELEASING) and I will merge this PR right behind it? |
|
What are we paying you for @tejashah88 ;) |
ajcrites
force-pushed
the
force-chain-134
branch
from
2a839a3
to
d21c406
Compare
This required some updates to the core code to make sure the promise chain and corresponding values are always passed appropriately
Also updated library to ignore value returned from handler as this would be considered an exception to response.send
ajcrites
force-pushed
the
force-chain-134
branch
2 times, most recently
from
1dea61a
to
244cad9
Compare
|
I'm merging this. |
This completes the change for #134. Asynchronous support is no longer backwards compatible, and the
return falseand callbacks to handlers have been removed. Now, returning Promises from handlers is required for asynchronous support.preandpostnow both also have asynchronous support. The API functions identically as before as long as you use the Promise chain for asynchronicity.response.sendandresponse.failcan force immediate success or failure..postcan still recover failures. Theresponse.sendandresponse.failmethods return promises that must be returned from inside the promise chain in order to continue it.Also closes #22