-
Notifications
You must be signed in to change notification settings - Fork 7
Promises/A+ compatibility and jQuery 1.8.3 version (mostly) #11
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
base: master
Are you sure you want to change the base?
Conversation
…better Promises/A+ compliance
- This preserves backwards compatibility - This makes the unit tests pass =]
|
I also bumped the version to 0.2.0, so if you're willing to accept it you should be able to just merge and then push to NPM. We would like to use the npm version, so would appreciate speedy acceptance or response with things to fix =] Thanks! |
|
So I'm a fan of this. I don't have time to fully look at the diff right now- but do these changes introduce breaking changes to the API? Or are they simply additive? I ask because we use this module heavily and we "might" not have the greatest hygiene on semver's in our package.json's. Any breaking changes within the same major version would break a lot of our modules. |
|
They definitely do not break our system, which uses this extremely heavily, and they don't break the unit tests; I recommend that you look at the unit tests and see if there are any patterns that you use that aren't covered and add tests. I'm about 95% sure that this won't break anything, but I can't 100% guarantee that. The API should still be mostly the same; the main change is that .then now acts like .pipe instead of returning the same object. This is in accordance with the Promises/A+ spec, and it's a change from jquery 1.8.3, but it is a change. You'd have to try it out to be sure; I'd say anywhere you're using .then is the most likely you might break. |
|
@ryanstevens this has the change that causes "then" to act the way "pipe" used to act. Then effectively creates a new promise where the result of the "done" callback is treated as the result. When we updated to the newer jQuery in the client we had bugs from this but they were wasy to mitigate. You should be fine to just roll through the references to then (you probably don't have many) and ensure that what you are returning from them is sensible to persist the existing deferred or value to future callers. |
|
Any status on this? |
|
I don't really have a dog in this fight anymore (NOT that implying this PR is a fight by any means), I've since moved to another company. Maybe just do a major version bump to 1.0.0, if it's not 100% backwards compatible then you should do that anyway. |
|
That seems reasonable to me |
This change adds compatibility with other Promises/A+ libraries. Large portions of it were copied and pasted directly from https://github.com/wookiehangover/underscore.deferred, which is the other node.js port of jQuery Deferred objects. I did keep their tweak (described on their page) of the "then" behavior to be more compatible with other promises/A+ libraries and I added back in a couple of removed (deprecated by jQuery) methods in order to preserve backwards compatibility and make sure all tests pass.
I also added some unit tests for interoperability with other "thenable" Promise libraries, including the ones used by Mongoosejs, Q, v8 (they are already built in on Chrome), and any other Promise/A+ library.