-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Making error/success handlers promise compatible #2221
Conversation
}; | ||
options.error = wrapError(options.error, model, options); |
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 this
instead of model
would make more sense for these 4 options.error =
statements, especially given the following line uses this
.
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 agree, updated.
I like it 👍 This make's |
Fantastic, this is a great way to do things. I shot you a quick pull request with a couple of tweaks, but otherwise I think this is a nice improvement that will solve several issues. |
Add missing wrapError and simplify a bit.
@caseywebdev Even if the repo isn't in the dropdown, you can still use the url for the pull. \o/
|
Proper PR sent! |
One simplification step further...
Added some tests to ensure the |
@jashkenas thoughts on this? I feel this makes |
Semicolon.
Making error/success handlers promise compatible
Any idea when this will go live? |
It is already in the current master branch. Just try the latest Backbone Edge build. |
I did, and it works. Just curious as to when the stable version will include this, since I don't want to use the Edge built in production as it may have changes that are not 100% tested yet. |
I don't think I made a solid enough argument in my last request for reverting the changes in #2003 and #1599
The Promises spec provides one of the best approaches to handle async calls, such as is needed with data syncing - (jQuery's ajax uses an almost Promise/A compatible deferred object internally).
The initial request of #2003 was to standardize the arguments passed back to the success/error handlers, but the merged request ended up requiring that the options handlers pass the model and the options through the sync implementation.
While the changes in #1599 made the code a little dry-er, it pushed the success/error handling logic into
Backbone.sync
. This then requires that the developer replaceBackbone.sync
and also replicate this logic if they wish to use their own syncing mechanism. While the change cuts a few lines internally, moving this logic out of the model/collection methods and into a separate component makes theBackbone.sync
more tightly coupled with the models/collections. This pull request puts the logic back into the model/collection methods, making the success/error handlers suitable for use as stand-alone deferredresolve
orreject
handlers, respectively.Here's an example of what can be accomplished when the previous (0.9.2 and earlier) sync structure is in place using q.js, a great library for implementing promises:
The same behavior would require much more work to implement when needing to reproduce the
success
/error
triggers, and passing through themodel
&options
.Unlike the other request, this does not re-attach
wrapError
as a property onBackbone
(keeps the global namespace clean), and it fulfills the original intent of #2003 which was to standardize the Backbone success/error handlers with(model, resp, options)
.