Closed wizardwerdna closed 10 years ago
Can you provide a link to the 2.x suite of tests and perhaps the result of running $q against it?
Its all at https://github.com/promises-aplus/promises-tests. The easiest way would be to delete node_modules/promises-aplus...., modify package.json version number to "*" and run a fresh npm install.
Once you do that, run the tests using grunt, but before you do, be sure to update the "promises-aplus" adapter, the form of which has been modified. (Essentially, changing fulfill and fulfilled to resolve and resolved -- I set up my adapters to run with both forms of the suite.)
Before you change the adapter, every test will fail. After you change the adapter, a kazillion tests will fail. The key changes that account for most of the errors are:
(1) callbacks must be called with the global object in loosey-goosey mode and undefined in strict mode as this. (2) Foreign thenables must be especially handled and called in the resolve process, per the new 1.1 promises-aplus spec. The process entails: (i) assuring that promise.then is called exactly one time for each potential promise object (to protect against thenables with getters and setters returning different values; (ii) assuring that the "then" is called with resolve/reject for the handling promise in a particular way, and that at most 1 of resolve/reject is called, and then at most one time (this is trickier than it seems); and (iii) the foreign thenables are flattened until a value is reached (possibly resulting in a loop in willingly obnoxious cases). For (iii), you are entitled to attempt loop detection and MAY reject in that case, but you are required to handle self-resolution (for both native and foreign promises), and MUST throw a type-error in that case.
To that end, efficiency demands that you handle native promises separately, since all they require is performing a p2 = p.then(p.resolve, p.reject) (with appropriate bindings) and you are done with the resolve.
The current $q is a bit of a mess with an eye toward those changes. I am already working on a refactoring for that, which has several benefits: (i) it has a single core object which does not include all the code in each instance; and (ii) permits a decent separation of concerns, permitting refactoring of repetitive common code.
For obvious reasons, I am retaining so much of the existing code as I can, although a total rewrite would be easier (when isn't it?) The external API will (as it must) be identical (I am adding one function right now) with identical semantics, and the existing tests will all pass.
Once I have the refactoring done to satisfy the existing test suite, I will start the 1.1 (2.x test suite) changes. I've done a few suites like this already (covenant and D.js), and think that once refactored, the upgrade should be simple.
Current footprint is more than 750 bytes of stack per promise, and should be reduced to less than 50 with this approach.
On Fri, Dec 6, 2013 at 2:28 PM, Pete Bacon Darwin notifications@github.comwrote:
Can you provide a link to the 2.x suite of tests and perhaps the result of running $q against it?
— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/5223#issuecomment-30035039 .
Okay, so I'm going to take this on I guess!
First things first, getting tests passing the v2.0.3 suite. So far, 323 are passing, and 500 are failing. Some of these seem pretty trivial to fix, so I'll try to get those working, and then we can look at putting all the proper functionality into a prototype.
This could take me a while, since there are a lot of failures :(
@wizardwerdna if you'd like to help I'm happy to collaborate --- The big block of failures is in the 2.3.3 suite of tests :(
I can pass along some guidance as to key areas to give attention. In the 2.x suite there are many tests for each issue, so you might only have a few failing issues:
1) must handle foreign thenables in a very particularized way a) must call the then at most one time, because the then could be a getter. So you must save the result of the get. b) must assure that the callbacks/errbacks to the then, only one is ever called, and then at most once. They do CRAZY things to senak around this, so you must code carefully. For starters, I would recommend writing a functor that takes each callback and errback and buries it in a function (i called mine once) that assures at most one call. (Check out the code in Covenant for an example). Not only must you assure at most once to the callback and errback, but you must also assure that this applies to the failure throw case as well. c) the code for the foreign thenable handling is expensive and slow, so you will want to identify trusted promises, which will require a bit of a trick from the existing code, which doesn't really have constructors to identfy same. If you can distinguish them, you can do the entire resolve by simply executing d.resolve(p); d) you MUST throw an error for d.resolve(d.promise)
If you nail these four issues, I would be surprised if you aren't finished. Indeed, if you think you have messages not related to them, I would read carefully to make sure (as the messages are pretty weak).
If you would like another set of eyes, please advise. I'll sit back on the project until you weigh in.
On Mon, Dec 16, 2013 at 12:44 PM, Caitlin Potter notifications@github.comwrote:
Okay, so I'm going to take this on I guess!
First things first, getting tests passing the v2.0.3 suite. So far, 323 are passing, and 500 are failing. Some of these seem pretty trivial to fix, so I'll try to get those working, and then we can look at putting all the proper functionality into a prototype.
This could take me a while, since there are a lot of failures :(
— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/5223#issuecomment-30698416 .
Alright how about we do this, I'll push my current WIP stuff to github, and we can hack on it to try and get the remaining tests passing. Currently I'm only running the 2.3.3 suite because it is the biggest source of problems, and I've got about 30 of them passing (at minimum, since I've xdescribe'd most of the suites so that they're easier to manage) -- But my changes to the test suite itself obviously won't show up on github, heh. Give me a few minutes and I'll push that stuff up if you'd like to take a look. For sure any changes that I make to $q will need to go through some serious review before it's landable
https://github.com/caitp/angular.js/tree/aplus203 << my work-in-progress hmm, it might actually be easier to start totally from scratch on v2.0.3 stuff, but I'll take another look tomorrow.
Will do. I tried that approach iniitally, and decided it needed a serious refactoring before meaningful change could be effected. Let's try it your way again, now that i've done some work on its internals.
On Mon, Dec 16, 2013 at 4:19 PM, Caitlin Potter notifications@github.comwrote:
Alright how about we do this, I'll push my current WIP stuff to github, and we can hack on it to try and get the remaining tests passing. Currently I'm only running the 2.3.3 suite because it is the biggest source of problems, and I've got about 30 of them passing (at minimum, since I've xdescribe'd most of the suites so that they're easier to manage) -- But my changes to the test suite itself obviously won't show up on github, heh. Give me a few minutes and I'll push that stuff up if you'd like to take a look. For sure any changes that I make to $q will need to go through some serious review before it's landable
— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/5223#issuecomment-30715233 .
Unsure if this is relevant or right place for this point but native promises are coming to JS - and with a different API than $q.
Refer to the native Promise implementation here: http://www.html5rocks.com/en/tutorials/es6/promises/.
Would it be appropriate to adopt the native API to be more future proof?
We've talked about that a bit today @hswolff, but I guess it's probably going to be a while before we can really delegate things to the native es6 implementations (although definitely, that would be awesome)
Its a fine place to begin the discussion. Yes, native promises are coming, and they are largely Promises/A+ 1.1 compatible. So bringing $q up to speed would be a big win. Also yes, there is a different API presented, but they are trivially matched with an adapter. For example, you can build a convenience function for defer whenever you new up a traditional promise object.
function defer(){
var deferrable;
new Promise(function(resolve, reject, progress){
deferrable = {
promise: this,
resolve: resolve.bind(this),
reject: reject.bind(this),
progress: progress.bind(this)
}
}
return deferrable;
}
You can likewise produce an adapter for promise objects, and you should be 90% home.
On Mon, Dec 16, 2013 at 7:03 PM, Caitlin Potter notifications@github.comwrote:
We've talked about that a bit today @hswolff https://github.com/hswolff, but I guess it's probably going to be a while before we can really relegate things to es6 functionality (although definitely, that would be awesome)
— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/5223#issuecomment-30722677 .
Related to native promises: Wondering if Angular would ever support a similar type of API:
var promise = $q.promise(function(resolve, reject) {
//...
});
@ewins I'm a fan of the resolver function promise API.
@wizardwerdna that PR is passing the aplus tests, but unfortunately it breaks some of the extra API, and it looks like using a strategy similar to Q to unbreak it would add a significant amount of code :( when you get some time it would be great if you could poke through it and see what it could do better
I will do. Find it odd that we are in this situation -- my experience is that a tight core makes the rest simpler. I'll do some surgery this week.
Best.
On Thu, Dec 26, 2013 at 11:35 AM, Caitlin Potter notifications@github.comwrote:
@wizardwerdna https://github.com/wizardwerdna that PR is passing the aplus tests, but unfortunately it breaks some of the extra API, and it looks like using a strategy similar to Q to unbreak it would add a significant amount of code :( when you get some time it would be great if you could poke through it and see what it could do better
— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/5223#issuecomment-31232668 .
yeah, it's a bit weird. It only seems to break the finally
tests, calling the handlers too soon I guess
Any update on this since January?
@DavidSouther @lgalfaso has a PR for updating $q which we need to land, that will probably not happen this week, but we want to get that in before shipping 1.3
Last time es6
-style promises was considered was in Dec (https://github.com/angular/angular.js/issues/5223#issuecomment-30722677). Since the specs are done and 3 major browser vendors already implemented it (http://beta.caniuse.com/#search=promise), does it make sense to switch to a polyfill with standard APIs like https://github.com/jakearchibald/es6-promise? I loved using $q
w/ angular for a long time, but perhaps it's about time.
@rayshan that's not going to happen in 1.x.
Work is already being done for this in angular 2.0 (https://github.com/jeffbcross/deferred was related to this, for example)
I see, that works, thanks for the update @caitp.
Closing as most of the original issues have since been addressed.
Given the importance of promises to the angularJS way, it is probably worth spending some time considering weaknesses in the $q models:
1) while compliant with much older versions of the promises-aplus spec, it is nowhere near complaint with the present 2.x spec and test suites. Since the 2.x suites are focused, in particular about integration of promises with foreign "thenable" libraries, this is actually pretty important.
2) $q promises are very heavyweight objects, taking more than 750 bytes of heap space per defer/promise pair. The main culprit here is that each promise contains a repeated and identical copy of all the functions used in the closure. This is one of the reason why most major promise implementations use prototypes in one way or the other to reduce the footprint. A flyweight version so implemented usually uses less than 10-50 bytes per instance.
3) There are a few features that are meaningful conveniences that require no space.
As understood, the primary motivations for the existing model is compactness and simplicity. I believe these features can be maintained while bringing $q up to date and flyweight, but it would require a major restructuring.
I'm inclined to volunteer for the project, but am curious if anybody else is looking at the issue, or wishes to chime in on requirements. Mine are:
1) identical API, perhaps with additional features (like a resolver instance for promise creation);
2) Flyweight implementation;
3) Conforms to most current promises-aplus specification (and hence far better integration with foreign thenables).