WIP STAGING for #8300 [[no-merge]] #8067
Conversation
| @@ -8,6 +8,37 @@ | |||
| * @description | |||
| * A promise/deferred implementation inspired by [Kris Kowal's Q](https://github.com/kriskowal/q). | |||
| * | |||
| * $q can be used in two fashions --- One, which is more similar to Kris Kowal's Q or jQuery's Deferred | |||
There was a problem hiding this comment.
Since it's an alternative way to use the API and it doesn't support all methods, it's documentation should come after $q, simply to prevent confusion.
There was a problem hiding this comment.
nah, it should come first IMO. Supporting "all the methods" would just mean adding an extra parameter to the resolver function.
Since this is basically what Promise APIs look like these days, it's actually saner to put this up front, with the older style further down --- and the older style should probably be eventually deprecated
| describe('$Q', function() { | ||
| var resolve, reject, resolve2, reject2; | ||
| /* jshint newcap:false */ | ||
| var Q = function() { |
There was a problem hiding this comment.
calling this helper function Q is a bit misleading. how about just createPromise or something that tells the test reader that the function call is a test helper?
|
|
||
|
|
||
| $$proxyPromise: function Promise$$proxyPromise(promise) { | ||
| if (this.isResolved()) throw new Error('Cannot proxy for resolved promise'); |
There was a problem hiding this comment.
These assertions were just used for debugging, it would be helpful to keep them but they probably won't look like this.
The promise-proxying strategy of Bluebird is a keeper, I think. Promise$$cast and $$proxyPromise have been helpful.
|
so the current state of things is, aplus tests are passing (I think these are still old versions of the aplus tests though), however many angular tests are failing due to issues with the promises. Some of those have been sorted out (deferred objects and promises must now be called with their correct context, so $http had to change slightly), but I'm still sorting through the broken ones to see what needs to be done. |
|
I don't see any benefit in making all of these $q changes in one PR. There is one commit that's nearly ready to go, but is blocked by being merged because of a big experimental refactor. @caitp can we separate this into multiple PRs, and create a detailed issue enumerating the improvements we intend to make with the refactor?
|
caitp
changed the title
|
@juliemr I think this is currently breaking something in how protractor interacts with the app, because as far as I can tell the tests which are failing are working as expected. Can you think of anything which would causes issues there? |
|
@caitp I don't see an obvious cause. I can take a longer look tomorrow, but it's odd that it's only that one test which is failing. We should figure out if it's a synchronization issue by putting in a |
|
As far as I can tell this is a real bug. Go to http://localhost:8000/build/docs/examples/example-$route-service/ and try clicking the different links - the text in the view doesn't always update. Note that the bindings below the view update, but the view itself doesn't. |
|
When I was testing, I would always see the animation run and the UI update as expected. hm... |
|
Thanks for looking into it, I'll try and figure that out I guess |
|
CC @petkaantonov: @caitp is experimenting with swapping out some of (all of?) the And thanks for the comments and emails @benjamingr! |
Things remaining: 1. Make this compatible with the original $q implementation 2. Sort out the error logging story (make use of Bluebird's machinery for this + $exceptionHandler by default) 3. Remove more bits that aren't needed for Angular.
|
So, as I mentioned in the dev-chat, the problem which was breaking this previously was that some ngRoute e2e tests were failing due to depending on unbounded execution of Deferred.resolve() --- I've added support for this back in to fix those tests, and I think that should fix all of the breaking changes that would be introduced by this patch (I've re-enabled tests which were previously disabled earlier in the prototype due to breaking exactly that aspect). So I think this is good for a more proper review so we can look at the parts we want to get rid of and the parts we want to hang onto. |
|
Welp, Igor didn't like this one, so closing it in favour of #8437, will sort out the remaining issues later |
Staging ground for $q improvements in #8300, this is a work in progress.
The work is in 3 stages, each building on the previous stage:
This is somewhat challenging, as promises in $q currently behave somewhat inconsistently (sometimes rethrowing errors, sometimes not, etc), and it will take some time to finalize this. Hence, staging ground.