refactor($q): make $q Promises A+ v1.1 compilant #5976
Conversation
|
Ah you're having a go at this as well :) I've been working on this a bit, it's quite challenging, I'd be happy to work with you on it |
|
@caitp had to redo part of the current implementation to follow the spec, hope that the refactor makes is easier to understand how everything works. If you find anything that this is failing at, or if this is not working as expected, please let me know and I will look into it. Merge with trunk as it looks travis does not like 02c88f9 while running |
|
sweet!! good work @lgalfaso I'm scheduling this for 1.3. |
|
|
||
| var $q = qFactory(process.nextTick, function noopExceptionHandler() {}); | ||
|
|
||
| exports.fulfilled = $q.resolve; | ||
| //exports.resolved = $q.resolve; |
There was a problem hiding this comment.
It was already there, but given that there is no $q.resolve I commented out. Will delete it and rebase
ghost
assigned
| promise: { | ||
| then: function(callback, errback, progressback) { | ||
| then: function(onFulfilled, onRejected, progressback) { |
There was a problem hiding this comment.
can we also rename progressback to follow the new naming?
|
I did a quick superficial review. It looks good. we should thoroughly review the change and merge it in in 1.3. @caitp can you own this please? |
|
Alright |
| @@ -188,6 +188,23 @@ function $QProvider() { | |||
| */ | |||
| function qFactory(nextTick, exceptionHandler) { | |||
|
|
|||
|
|
|||
| function callOnce(elements) { | |||
There was a problem hiding this comment.
This doesn't look like it's going to help us save on memory at all...
But without worrying about that, maybe this could be a bit more self-documenting.
I think this could probably perform slightly better if an array was not being created each time the function was called, and was not being grown with push(). Why not always assume that there are two indices (since that's how many it's called with), and just use ordered parameters.
return [
wrap(resolveFn),
wrap(rejectFn)
];This would get rid of the oddly named variables which make the function a bit harder to follow
There was a problem hiding this comment.
But yeah, using this closure means creating at least 3 closures every time resolve() is called, and that just feels wrong to me
There was a problem hiding this comment.
You are right, will follow the pattern you proposed.
|
01a909f reduces the memory footprint (the diff looks big, but it mostly moves the existing code). It is possible to reduce the memory footprint a little more, but will do so at the expense of readability |
|
@lgalfaso personally I think we should just try to ship the aplus 1.1 passing code before trying to fix up the memory stuff |
|
@caitp ok, do you want me to remove this last commit from the branch? |
|
you should probably hang onto the commit, maybe in a separate branch, but in my opinion yes it should not be included as a part of this PR. |
|
@caitp ok, done |
|
@lgalfaso do you want to rebase so that it's easier to land this in a few weeks? 1.3 is coming up fast |
|
Just rebase everything, but it looks like master is broken as there is some issue with the docs generation |
|
okay, lots of comments --- I apologize if I sound snotty in some of those, it's not intensional -- this is good stuff, just needs a little bit more work =) |
|
@caitp comments were good. Updated the patch with the feedback. |
|
Sorry, and the discussion on the extra |
|
I don't think it's necessary to fix all of those complaints in this PR, but my worry is that if they don't get addressed, they'll just be forgotten about. Mostly I just want to shrink the diff by avoiding the small/meaningless changes that aren't really needed |
|
I am ok with fixing all the issues before the PR is merged |
(answering here because it's way too hard to find that comment) --- no, there's really no need to wrap the functions like that. the tests will only fail if we invoke the handlers more than once, which we can avoid doing without wrapping the functions and creating more garbage |
We have to do something to be sure that resolve is only called once, just like https://github.com/petkaantonov/bluebird/blob/master/src/thenables.js#L61 |
|
Not quite --- what happens is, when settling a promise, we invoke a handler --- if the return value of the handler is a thenable, we may wrap it --- but if it's an instance of our own promise class, we have a fast path which is used instead. In angular, most of the time we aren't dealing with third party promises, so we want to use the fast path as frequently as possible --- for this path, none of this matters |
Ok, now I get what you are saying. Will update the PR with this in place |
Refactor to make $q Promises A+ v1.1 compilant * Rewrite the promise resolution mechanism * Remove inline functions from `then` * Keep and explicit state of the promise
|
@caitp made the changes to fast-track then it is a promise we know about. Please let me know if you have any other comments for this patch |
|
It's probably good enough for now, for me. It would be good for someone else to review as well --- it's possible a lot of my opinions will be disagreed with --- @rodyhaddad want to take a look when you have time? |
|
@rodyhaddad are you still planning to take a look at this? |
|
@jeffbcross I don't have much to add to it. I believe that in the 1.x meeting on Monday, @IgorMinar said he'll take a look at it |
|
I think Igor had mentioned there was too much happening in a single commit, and that it should be split apart (am I misremembering who said that?), if possible. |
|
@caitp that sounds right. Other notes I remember:
|
|
@lgalfaso do you want to make the suggested changes (splitting commits betweeen A+ and perf improvements with more explicit A+ explanation, using prototype literal)? If you don't have time, I can help. |
|
I'm around to help with this too if you're busy Lucas |
|
@jeffbcross @caitp if you can split this into several commits that take care of the different parts that made this PR, I will be thankful |
|
Landed 1339c11 |
Refactor to make $q Promises A+ v1.1 compilant