Closed
Bug 708984
Opened 12 years ago
Closed 12 years ago
Promote Promise.jsm from developer tools to toolkit
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: jwalker, Assigned: irakli)
References
(Blocks 1 open bug, )
Details
Attachments
(3 files, 8 obsolete files)
367 bytes,
text/html
|
mossop
:
superreview+
Yoric
:
feedback-
|
Details |
10.89 KB,
patch
|
Details | Diff | Splinter Review | |
7.64 KB,
patch
|
Details | Diff | Splinter Review |
Developer tools have an implementation of Promises [1]. Which is useful to the main-thread-io work in Bug 692420. It seems sensible to share. Our logic in initially using Promise was: "We need something to help us write async code. Promise is simple, widely understood and works everywhere. Better language level solutions are on the horizon, but they're not here yet" This decision is easy to make in a very localized context, but should be considered more widely if we're going to build significant dependencies on it. [1] browser/devtools/shared/Promise.jsm
Comment 1•12 years ago
|
||
As we need the feature, we really hope that you will share it. Otherwise, we will need to fork it, which would be a shame.
Reporter | ||
Comment 2•12 years ago
|
||
I think we can agree that it would be bad to fork this promise implementation, and also that it would be better if the main-thread-io work didn't use stuff from devtools. So it seems sensible to move the promise implementation somewhere shared. Before we create a patch to move this to toolkit I'd like: * confirmation that the promise approach is ok, that there isn't a nice language level feature that we could/should consider * to know where to put it So I added Mossop/Brendan to the CC list. It looks from the ecmascript:strawman on concurrency that Q is smiled upon. I was hoping for something with more language sugar, or maybe this all too far away.
Comment 3•12 years ago
|
||
(In reply to Joe Walker from comment #2) > It looks from the ecmascript:strawman on concurrency that Q is smiled upon. > I was hoping for something with more language sugar, or maybe this all too > far away. Since Allen is the author (or one of the authors) of the Strawman proposal, I add him to the Cc. However, just to be sure I understand: are you suggesting that we should implement Q rather than using your existing Promises.jsm? Or that Promises.jsm may eventually be reimplemented on top of Q?
Reporter | ||
Comment 4•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #3) > However, just to be sure I understand: are you suggesting that we should > implement Q rather than using your existing Promises.jsm? Or that > Promises.jsm may eventually be reimplemented on top of Q? I'm not really saying either, but concious that some may have a (possibly strong) opinion that Q is a better solution. For what it's worth my opinion on async is that I like a solution that is: simple and works on modern browsers, today (hence Promises). But that's just me. Other forces are at play and I'd rather let them speak for themselves.
Comment 5•12 years ago
|
||
My personal take on this: - if we notice anything in Promises that could be broken at a later stage by the introduction of Q, rewrite this little bit and refactor everything that depends on it; - once this is done, move Promises to somewhere inside toolkit and officialize the fact that it is public; - once we have Q, reimplement Promises on top of it.
Comment 6•12 years ago
|
||
I know basically nothing about Promise or Q. Any useful docs you can point me to?
Comment 7•12 years ago
|
||
dherman blogged about his work with promises today: http://calculist.org/blog/2011/12/14/why-coroutines-wont-work-on-the-web/
Reporter | ||
Comment 8•12 years ago
|
||
(In reply to Dave Townsend (:Mossop) from comment #6) > I know basically nothing about Promise or Q. Any useful docs you can point > me to? # Promise http://wiki.commonjs.org/wiki/Promises/A https://hg.mozilla.org/mozilla-central/file/5727a8f457c1/browser/devtools/shared/Promise.jsm # Q https://github.com/kriskowal/q http://wiki.ecmascript.org/doku.php?id=strawman:concurrency Personal opinion: Q has a 'mathematical' feel to it. Elegant, avoids some theoretical problems, but non-simple.
Reporter | ||
Comment 9•12 years ago
|
||
The summary of this bug, for me, is: Since no-one has said: 'The correct solution is X', then momentum wins, and we continue with using Promises.
Comment 10•12 years ago
|
||
Nice, I like it. Actually it reminds me of something I was considering returning from all the AddonManager async methods however I also added the idea of a ".cancel()" call on it to say that you're no longer interested in the result of an async operation, f.e. when the UI that was waiting for data is closed. Don't know if that is of interested too, I never got around to implementing it.
Reporter | ||
Comment 11•12 years ago
|
||
Thanks. Could you advise a location?
Comment 12•12 years ago
|
||
toolkit/content is probably the quick n dirty place to stick it. We don't really have a better place for shared modules right now.
Comment 13•12 years ago
|
||
How about putting it in toolkit/modules, like we're doing for broad-use modules in /browser?
Reporter | ||
Comment 14•12 years ago
|
||
Or to add confusion, devtools shared modules go in browser/devtools/shared. Geometry and Dict feel vague utility like, so toolkit/content doesn't feel 'wrong' to me.
Comment 15•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #13) > How about putting it in toolkit/modules, like we're doing for broad-use > modules in /browser? I'd be fine with this, and moving the existing shared modules there if you like but I don't think it's worth bikeshedding on.
Reporter | ||
Comment 16•12 years ago
|
||
(In reply to Dave Townsend (:Mossop) from comment #15) > (In reply to Dietrich Ayala (:dietrich) from comment #13) > > How about putting it in toolkit/modules, like we're doing for broad-use > > modules in /browser? > > I'd be fine with this, and moving the existing shared modules there if you > like but I don't think it's worth bikeshedding on. I'm OK with this so long as I'm not expected move anything but Promise :-)
Comment 17•12 years ago
|
||
It doesn't really make sense to create a new spot for modules and then not put the existing modules in it. We don't need to block this bug on creating a new spot for modules, though. toolkit/content is fine for the moment.
Updated•12 years ago
|
Product: Core → Toolkit
QA Contact: general → general
Updated•12 years ago
|
Summary: Promote Promise.jsm from developer tools to browser/platform → Promote Promise.jsm from developer tools to toolkit
Comment 18•12 years ago
|
||
Sounds like moving the file to /toolkit/content will do for now. Anything else blocking this from happening asap?
Comment 19•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #18) > Sounds like moving the file to /toolkit/content will do for now. Anything > else blocking this from happening asap? Looks like this module never had a sr pass so I think we should correct that. I know some of the add-on SDK team had some comments about the current design of the module too.
Comment 20•12 years ago
|
||
Actually, Irakli had some comments. The current implement isn't directly compatible with Jetpack API, as we need to separate `then` and `resolve` as we do not want addons to fullfill promises. `resolve` has to be kept private and be called only by private SDK API/code. So that we can't return `Promise` instances from our public APIs. Irakli asked me to review the following implementation: https://github.com/mozilla/addon-sdk/pull/335 Unfortunatly, I didn't had time to do it before this bug get discussed. But I can increase priority of this review if it can help.
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Dave Townsend (:Mossop) from comment #19) > I know some of the add-on SDK team had some comments about the current > design of the module too. Yes I had some comments on the current implementation of promises which I discussed with Joe in person and was going to suggest alternative, but unfortunately review for it is still pending. That being said documentation & code can be viewed in this pull request (Alex you mention a wrong one): https://github.com/mozilla/addon-sdk/pull/350/files As of my comments please see them below (P.S.: Don't mean to criticize Promise.jsm) 1. One of the most important features of promises is an implicit error propagation, that works just like try, catch, and finally. Error in step 1 will flow all the way through promise chain until step N, where it’s caught by an error handled. Unfortunately this is not true for the `Promise.jsm`. 2. Another important feature of promises is that they allow easy chaining of promises. It may be via `then` which according to CommonJS returns promise `foo.then(function() { .. }).then....` or via promise resolution with other promise, in which case resolution value propagates similar to errors. This is not quite as simple with `Promise.jsm` as it has special helper function to do that kind of thing. 3. In many (in fact in all cases for SDK) promise consumer and promise fulfiller represent different parties and it's extremely important for us to just provide access to one or another since consumer is untrusted and those should not have a way to fulfill promise and trick other consumers. As of solution in mentioned pull request, I'd say it's a very minimalistic subset of Q (mainly because we just need bare-bones of Q).
Assignee | ||
Comment 22•12 years ago
|
||
BTW CommonJS Promises/A becomes de facto standard, as it's supported by: [Q](https://github.com/kriskowal/q/blob/master/q.js) [Dojo](http://www.sitepen.com/blog/2010/05/03/robust-promises-with-dojo-deferred-1-5/) [windows 8](http://msdn.microsoft.com/en-us/library/windows/apps/hh700330.aspx) [jQuery](http://api.jquery.com/category/deferred-object/) [node-promise](https://github.com/kriszyp/node-promise) [task.js](http://taskjs.org/) So compliance with it is must have IMO.
Good that we finally have this conversation. As a user of Promise, I personally have no issue moving to another, more standard, API, provided that: - this API can be used both from xpcom, xul content and chrome worker; - this API offers me the ability to schedule promises for execution through the use of |setTimeout|/|nsITimer|. I am, of course, willing to contribute some of these features, if necessary. A few notes: - I personally like our implementation of |trap|, which is sometimes much clearer than using |then| when compartmentalization of errors is required; - at the moment, Promise reports uncaught errors, and can (in debug mode) capture a stack during creation - quite useful for debugging.
Reporter | ||
Comment 24•12 years ago
|
||
If we're going to radically change the API then: - We should consider starting afresh in toolkit rather than updating all the uses in devtools. If it's easy to update fine, but some of these changes sound radical - The one extra feature I wanted was some sort of progress system, which IIRC yoric wanted too, however we've yet to get this agreed, and maybe there is no good general solution - We previously ruled 'Q' out, but if we're talking standardized, then perhaps we should consider it more?
Assignee | ||
Comment 25•12 years ago
|
||
David, Joe I personally favor very minimalistic API that we're in the process of landing to SDK. It does only few things but does them well. It's a subset of Q, simpler and smaller. In the current patch it provides only 4 functions and is about 208 lines (more than half is comments) while Q provides 40 functions and 970 lines of code (can't say how much of that is a comments). As a matter of fact I'd encourage you to look at the documentation: https://github.com/Gozala/addon-sdk/blob/4211538044a140c5f06a81179aad4815d88e698d/packages/api-utils/docs/promise.md And maybe even implementation: https://github.com/mozilla/addon-sdk/pull/350/files#diff-1 And let me know if that could satisfy your needs. As of particular feature requests, this implementation is very small and mainly provides two core functions: defer and promised. I won't go into details as documentation contains all of them but will say that both functions take optional `prototype` argument that will be used as a `prototype` for the returned promise. This would allow anyone to implement any kind of methods like `trap` or others that `Promise.jsm` does and much more. As of `progress` handling in promises, I can argue that it's a very bad idea as semantics of promises are not much for that. On the other hand promises may be used to implement streaming data structures. As a matter of fact I did that: https://github.com/Gozala/streamer/wiki/stream https://github.com/Gozala/streamer/blob/master/core.js P.S: Kris behind Q is actually studding it and agreed with me that this is a better direction. I will also make sure that our promise implementation can be loaded as jsm or used directly from script tag in window.
Irakli: I am a little wary of mixins, but yes, I will take a look at your code.
Assignee | ||
Comment 27•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #26) > Irakli: I am a little wary of mixins, but yes, I will take a look at your > code. Oh no, we no do mixins. Only thing promise spec defines is a `then` method and that's only thing our promises implement & we don't care what the prototype of promise object we will return will be, if you give us one we use that otherwise we fallback to Object.prototype: https://github.com/mozilla/addon-sdk/pull/350/files#L1R86 It's somewhat similar to Object.create
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #27) > (In reply to David Rajchenbach Teller [:Yoric] from comment #26) > > Irakli: I am a little wary of mixins, but yes, I will take a look at your > > code. > > Oh no, we no do mixins. Only thing promise spec defines is a `then` method > and that's only thing our promises implement & we don't care what the > prototype of promise object we will return will be, if you give us one we > use that otherwise we fallback to Object.prototype: > https://github.com/mozilla/addon-sdk/pull/350/files#L1R86 > > It's somewhat similar to Object.create And how is that not a mixin? :) (In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #25) > As of `progress` handling in promises, I can argue that it's a very bad idea > as semantics of promises are not much for that. On the other hand promises > may be used to implement streaming data structures. As a matter of fact I > did that: Not 100% sure about progress handling in promises, but currently, I think that it is actually completely dual to promise. A promise is one result, while progress is a stream. This would be just as ugly as, you know, having a language in which functions can yield instead of returning a result :) > I will also make sure that our promise implementation can be loaded as jsm > or used directly from script tag in window. Good. > This would allow anyone to implement any kind of methods like `trap` or others that `Promise.jsm` does and much more. Well, I am not worried about implementing |trap| & co., that is quite trivial. I am more worried about the following: - ensuring that chaining plays nicely with JS cooperative pseudo-concurrency, i.e. that it calls |setTimeout| or the corresponding XPCom machinery (see bug 692420); - ensuring that uncaught errors are reported (see bug 720628). (In reply to Joe Walker from comment #24) > - We previously ruled 'Q' out, but if we're talking standardized, then > perhaps we should consider it more? I second Joe's question. You are advocating standardization, which is good, but then suggesting something that, if I understand correctly, is non-standard. Is it strictly a subset of Q that we could simply replace by Q if/whenever Q is standardized?
Assignee | ||
Comment 29•12 years ago
|
||
> > And how is that not a mixin? :) > Maybe my definition of mixing is bit odd, I refer to an object as mixing if it inherits its functionality from more than one object, which is either by multiple inheritance or by manual property copying. In this case it's just regular inheritance with a similar API as provided by JS built-ins. > Not 100% sure about progress handling in promises, but currently, I think > that it is actually completely dual to promise. A promise is one result, > while progress is a stream. This would be just as ugly as, you know, having > a language in which functions can yield instead of returning a result :) > I'm happy we agree here :) > Well, I am not worried about implementing |trap| & co., that is quite > trivial. I just wanted to explain reasoning why pass in prototype is so crucial IMO and obviously `trap` is not the best example here. It's just bare promises are not enough for certain things like streaming data for example, but pass-in prototype allows one to build different data structures on top without reinventing a wheel. Another interesting use case is via proxy prototype and there are more. I'm happy to talk about this more on IRC or something if you're still doubtful. > > I am more worried about the following: > - ensuring that chaining plays nicely with JS cooperative > pseudo-concurrency, i.e. that it calls |setTimeout| or the corresponding > XPCom machinery (see bug 692420); > I don't see why there would be any issues with that, but if there is that would be an implementation bug. > - ensuring that uncaught errors are reported (see bug 720628). Any exception in any handler rejects underling promise and will propagate through the promise chain. So as long as errors are handled somewhere in the chain they won't get unnoticed.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #29) > > > > And how is that not a mixin? :) > > > > Maybe my definition of mixing is bit odd, I refer to an object as mixing if > it inherits its functionality from more than one object, which is either by > multiple inheritance or by manual property copying. In this case it's just > regular inheritance with a similar API as provided by JS built-ins. Not "mixing", "mixin". In OOP, a mixin is a class constructor parametrized by its superclass (or one of its superclasses). It is also a well-known mechanism for making a class very hard to understand :) > > I am more worried about the following: > > - ensuring that chaining plays nicely with JS cooperative > > pseudo-concurrency, i.e. that it calls |setTimeout| or the corresponding > > XPCom machinery (see bug 692420); > > > > I don't see why there would be any issues with that, but if there is that > would be an implementation bug. What I meant is that this is more important to me than customizing additional methods using a custom prototype. Let's discuss this on IRC, as you suggest. > > - ensuring that uncaught errors are reported (see bug 720628). > > Any exception in any handler rejects underling promise and will propagate > through the promise chain. So as long as errors are handled somewhere in the > chain they won't get unnoticed. Hence my focus on _uncaught_ :) Starting with bug 720628, Promise.jsm reports to the console errors that are never handled.
Assignee | ||
Comment 31•12 years ago
|
||
> Not "mixing", "mixin". Pardon my OSX Lion auto-correction, it's not very good at tech terms :) We can call it whatever but `defer(object)` is no different from standard `Object.create(object)` which was the best thing to happen to JS in long time :) Either way I wrote long post justifying this API in the pull request: https://github.com/mozilla/addon-sdk/pull/350#issuecomment-4872889 If that's still not convincing let's talk it over on IRC ;)
Assignee | ||
Comment 32•12 years ago
|
||
Now that our promises have landed to SDK: https://github.com/mozilla/addon-sdk/commit/ee0cbdec87c7daac69202df642ce051ef2e6414b I'd like to know if you'd be interested in using it as a replacement for Promise.jsm ? To give it a try you don't need an SDK you could just download module from: https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/promise.js And load it as JSM or as regular script tag.
Reporter | ||
Comment 33•12 years ago
|
||
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #32) > Now that our promises have landed to SDK: > https://github.com/mozilla/addon-sdk/commit/ > ee0cbdec87c7daac69202df642ce051ef2e6414b > > I'd like to know if you'd be interested in using it as a replacement for > Promise.jsm ? To give it a try you don't need an SDK you could just download > module from: > https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/ > promise.js > > And load it as JSM or as regular script tag. I just spent a while talking to Irakli about promises and related issues. My position is (still) that I think Q (or something Q-like) is a good idea, but that for developer tools to move to something like that would need there to be a consensus on the alternative. So if you buy into Irakli's proposal, Yoric, and we can persuade Mossop etc, then I'm all for it.
(In reply to Joe Walker from comment #33) > So if you buy into Irakli's proposal, Yoric, and we can persuade Mossop etc, > then I'm all for it. I am willing to go either way, it's your call.
Comment 35•12 years ago
|
||
Sounds like everyone is in agreement. Who's going to make a patch? Is it Mossop who needs to review?
Assignee | ||
Comment 36•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #35) > Sounds like everyone is in agreement. Who's going to make a patch? Is it > Mossop who needs to review? In that case I'll submit a new patch here and ask Mossop to review it.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #36) > (In reply to Dietrich Ayala (:dietrich) from comment #35) > > Sounds like everyone is in agreement. Who's going to make a patch? Is it > > Mossop who needs to review? > > In that case I'll submit a new patch here and ask Mossop to review it. ... And I will probably add a patch for error-reporting :)
Assignee | ||
Comment 38•12 years ago
|
||
At the moment we're trying to figure out how directory structure will look like with SDK in Firefox. This way hopefully we'll be able to maintain promise module under one repository. So I'm waiting to get more clearance on this https://github.com/mozilla/addon-sdk/wiki/JEP%20SDK%20in%20Firefox before submitting a patch for a review. Yorik meanwhile if you have some improvements for error-reporting you could submit them against https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/promise.js
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #38) > Yorik meanwhile if you have some improvements for error-reporting you could > submit them against > https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/ > promise.js Working on it. Note, however, that, in its current state, I rather fear that the implementation of promises does not match (at all) the criteria for inclusion in Firefox proper. I have left a few comments on github with suggestions on how to improve that.
Assignee | ||
Comment 40•12 years ago
|
||
Summing up some of discussions that happened off this thread: - Yorik is concerned that current implementation in SDK is not simple enough. - We managed to agree to preserve API and semantics of the current implementation. - Yorik forked SDK implementation with attempt to simplify it. - I don't agree that forked version is simpler, in fact contrary. I will try to get someone else look into both implementations to get less subjective input. - I'm currently occupied with our Jetpack Q2 goals, so promise related task are on queued. - I would like to try to [split promise abstraction](https://gist.github.com/2763402) into `Watchable`, `Eventual`, `Promise` as it may allow better interoperability as others could implement alternative `Promise` layer on top of lower level semantics provided by `Eventual`. - Upcoming ES.next private names (see Bug 645416) may be used to address most of the security concerns currently solved via closures. - It may be worth to polyfill them by using generated unique names to **maybe** simplify implementation. - Since I got Yorik on board for existing API and semantics it's probably best for him to start with his own fork and than update to promises implementation that we'll end up landing. Yorik feel free to correct if I god anything wrong.
Well, I beg to differ on the spelling of my nickname :) For the rest, I confirm that we have agreed on the following strategy: - for the moment, attempt to land on m-c my reimplementation of your promise API (or a variant thereof) and work from that code; - get other pairs of eyes to look at that code; - since we keep the API, we can later attempt to converge on a unified implementation. I have no opinion on the rest.
Assignee | ||
Comment 42•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #41) > Well, I beg to differ on the spelling of my nickname :) > Oops I'm sorry, I got too much used auto-complete on names in github. > I havUpcoming ES.next private names (see Bug 645416) may be used to address most > of the security concerns currently solved via closures.e no opinion on the rest. BTW I start to think about: > Upcoming ES.next private names (see Bug 645416) may be used to address most > of the security concerns currently solved via closures. As you wanted to avoid many closures, this would make it possible.
Attaching a version of promise based on a (simplified) version of the Jetpack implementation. I am not sure we need the final part (|promised|) for the moment, so we might want to postpone this part to an ulterior patch.
Attachment #630183 -
Flags: review?(jwalker)
Porting the companion testsuite.
Assignee: nobody → dteller
Attachment #630184 -
Flags: review?(jwalker)
Reporter | ||
Comment 45•12 years ago
|
||
I'm on holiday this week. Can this wait until next week?
Assignee | ||
Comment 46•12 years ago
|
||
>
> Attaching a version of promise based on a (simplified) version of the
> Jetpack implementation. I am not sure we need the final part (|promised|)
> for the moment, so we might want to postpone this part to an ulterior patch.
>
I still would like to verify that it's **simplified** for others as well. As I already pointed out, I don't find this implementation simpler, but contrary. Also I have looked into it close enough to verify that last draft actually implements exact same semantics (pointing this out, since all drafts I've looked into closely had differences).
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #46) > > I still would like to verify that it's **simplified** for others as well. Certainly. This is part of what I expect Joe to review. Do you have any other reviewer in mind?
Reporter | ||
Comment 48•12 years ago
|
||
Comment on attachment 630183 [details] [diff] [review] A new implementation of promises with an API compatible with that of Jetpack Review of attachment 630183 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your work on this Yoric. Part of the point of this exercise was to make something that was a subset of 'Q'. We don't define Q as an export, perhaps we should? Otherwise, aren't we asking people to do this? var Q = {}; Components.utils.import("resource:///.../promise.jsm", Q); ::: toolkit/content/promise.jsm @@ +16,5 @@ > + this.EXPORTED_SYMBOLS = Object.keys(this); > + } else { > + factory(undefined, (this.promise = {})); > + } > +})(function(require, exports) { I've a couple of modules that I support across JSM/browser via require/node, and I personally revolt more at the block above than the need to make smart updates to keep versions in sync. (That could be because my tools make this easy though). This isn't a change requirement, just a 'please consider'. @@ +78,5 @@ > + // something amiss. Perhaps we will want to log this, or perhaps we will > + // want to launch a setTimeout to refine this claim. > +}; > + > +function isPromise(value) { Is it worth having a utility for this? It's not exported (maybe it should be?) @@ +82,5 @@ > +function isPromise(value) { > + /** > + Returns true if given `value` is promise. Value is assumed to be promise if > + it implements `then` method. > + **/ I very much like the idea of python style inline comments, but one of the points of doc comments is uniformity across a codebase, so it's a bit strange having some modules being completely different like this. I'm not sure I care enough to argue, but it's not what I would do. @@ +90,5 @@ > +const STATE_PENDING = -1; > +const STATE_RESOLVED = 0; > +const STATE_REJECTED = 1; > + > +function defer(prototype) { I'm sorry to weigh in on a hotly debated topic. From what I can see Q doesn't support passing prototypes to defer(), so we're being a partial superset of Q. To me this calls into question its utility (Q hasn't exactly been slow to add features) @@ +124,5 @@ > + var foo = defer(prototype) > + deferred.promise.get('name').then(console.log) > + deferred.resolve({ name: 'Foo' }) > + //=> 'Foo' > + */ I think it would be a good idea to put the usage examples at the top of the file, save people searching for them. @@ +248,5 @@ > + var concat = Array.prototype.concat; > + > + // Utility function that does following: > + // execute([ f, self, args...]) => f.apply(self, args) > + function execute(args) { return call.apply(call, args); } Do we have evidence of an actual performance increase by doing this?
Assignee | ||
Comment 49•12 years ago
|
||
Comment on attachment 630183 [details] [diff] [review] A new implementation of promises with an API compatible with that of Jetpack Review of attachment 630183 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/promise.jsm @@ +90,5 @@ > +const STATE_PENDING = -1; > +const STATE_RESOLVED = 0; > +const STATE_REJECTED = 1; > + > +function defer(prototype) { I've talked with Kris Kowal about this feature over IRC & he was going to add it to Q. @@ +124,5 @@ > + var foo = defer(prototype) > + deferred.promise.get('name').then(console.log) > + deferred.resolve({ name: 'Foo' }) > + //=> 'Foo' > + */ Most examples and docs actually live here: https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/promise.md @@ +248,5 @@ > + var concat = Array.prototype.concat; > + > + // Utility function that does following: > + // execute([ f, self, args...]) => f.apply(self, args) > + function execute(args) { return call.apply(call, args); } I did bunch of profiling in web inspector (unfortunately jsperf was down for a couple of days), but we could probably do create jsperf test for it. But even without tests it's pretty clear that not creating closure on each call at line 283 makes a big diff.
Reporter | ||
Comment 50•12 years ago
|
||
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #49) > Comment on attachment 630183 [details] [diff] [review] > A new implementation of promises with an API compatible with that of Jetpack > > Review of attachment 630183 [details] [diff] [review]: > ----------------------------------------------------------------- > ... > > +function defer(prototype) { > > I've talked with Kris Kowal about this feature over IRC & he was going to > add it to Q. OK, thanks. That seems to be a potential future solution to one point, the other being about general utility - if there was a large clamour for this, wouldn't Q have it already? Also, we can add easily - it's a 10 line patch that we could all write in our sleep, but we can't remove it nearly as easily, or maybe at all. I vote we create a bug that asks for this to be added and attach a patch to it and maybe add a comment to the effect, and take a look at how much it's needed, and how Q gets on with it. > @@ +124,5 @@ > > + var foo = defer(prototype) > > + deferred.promise.get('name').then(console.log) > > + deferred.resolve({ name: 'Foo' }) > > + //=> 'Foo' > > + */ > > Most examples and docs actually live here: > https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/ > promise.md Is this the official home of addon-sdk docs? (Shouldn't the home be somewhat closer to MDN?) Anyway, I agree with the notion that we should probably link to the docs from the header. > @@ +248,5 @@ > > + var concat = Array.prototype.concat; > > + > > + // Utility function that does following: > > + // execute([ f, self, args...]) => f.apply(self, args) > > + function execute(args) { return call.apply(call, args); } > > > Do we have evidence of an actual performance increase by doing this? > > I did bunch of profiling in web inspector (unfortunately jsperf was down for > a couple of days), but we could probably do create jsperf test for it. But > even without tests it's pretty clear that not creating closure on each call > at line 283 makes a big diff. So if we're sure that it makes a difference, let's move them as close together as we can and kill call and concat. Personally I only sacrifice readability for performance with actual numbers relevant to the domain of use, but this is only one line so it's really not worth arguing about. Thanks Irakli.
(In reply to Joe Walker from comment #48) > Comment on attachment 630183 [details] [diff] [review] > A new implementation of promises with an API compatible with that of Jetpack > > Review of attachment 630183 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for your work on this Yoric. > > Part of the point of this exercise was to make something that was a subset > of 'Q'. We don't define Q as an export, perhaps we should? > Otherwise, aren't we asking people to do this? > > var Q = {}; > Components.utils.import("resource:///.../promise.jsm", Q); So, what would you suggest? In the context of a jsm, export a symbol Q containing function |defer|? > ::: toolkit/content/promise.jsm > @@ +16,5 @@ > > + this.EXPORTED_SYMBOLS = Object.keys(this); > > + } else { > > + factory(undefined, (this.promise = {})); > > + } > > +})(function(require, exports) { > > I've a couple of modules that I support across JSM/browser via require/node, > and I personally revolt more at the block above than the need to make smart > updates to keep versions in sync. (That could be because my tools make this > easy though). > This isn't a change requirement, just a 'please consider'. I am willing to consider anything :) However, I am not sure I understand what you suggest here. > @@ +78,5 @@ > > + // something amiss. Perhaps we will want to log this, or perhaps we will > > + // want to launch a setTimeout to refine this claim. > > +}; > > + > > +function isPromise(value) { > > Is it worth having a utility for this? It's not exported (maybe it should > be?) I believe that using this utility function in the implementation is more readable and more maintainable than introducing an immediate check for a method |then|. But yes, good point, let's export this. > > @@ +82,5 @@ > > +function isPromise(value) { > > + /** > > + Returns true if given `value` is promise. Value is assumed to be promise if > > + it implements `then` method. > > + **/ > > I very much like the idea of python style inline comments, but one of the > points of doc comments is uniformity across a codebase, so it's a bit > strange having some modules being completely different like this. > I'm not sure I care enough to argue, but it's not what I would do. I attempted to stick with what looks like Jetpack style (although I have not checked whether it is normalized). If you want, just say so and I will put it in javadoc/jsdoc style. > > @@ +90,5 @@ > > +const STATE_PENDING = -1; > > +const STATE_RESOLVED = 0; > > +const STATE_REJECTED = 1; > > + > > +function defer(prototype) { > > I'm sorry to weigh in on a hotly debated topic. From what I can see Q > doesn't support passing prototypes to defer(), so we're being a partial > superset of Q. > To me this calls into question its utility (Q hasn't exactly been slow to > add features) [...] > I vote we create a bug that asks for this to be added and attach a patch to it and maybe add a comment to the effect, and take a look at how much it's needed, and how Q gets on with it. Sounds good to me. > @@ +124,5 @@ > > + var foo = defer(prototype) > > + deferred.promise.get('name').then(console.log) > > + deferred.resolve({ name: 'Foo' }) > > + //=> 'Foo' > > + */ > > I think it would be a good idea to put the usage examples at the top of the > file, save people searching for them. Let's will do that. > > @@ +248,5 @@ > > + var concat = Array.prototype.concat; > > + > > + // Utility function that does following: > > + // execute([ f, self, args...]) => f.apply(self, args) > > + function execute(args) { return call.apply(call, args); } > > Do we have evidence of an actual performance increase by doing this? Let me suggest also postponing |promised| to a followup bug.
Reporter | ||
Comment 52•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #51) > (In reply to Joe Walker from comment #48) > > Comment on attachment 630183 [details] [diff] [review] > > A new implementation of promises with an API compatible with that of Jetpack > > > > Review of attachment 630183 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Thanks for your work on this Yoric. > > > > Part of the point of this exercise was to make something that was a subset > > of 'Q'. We don't define Q as an export, perhaps we should? > > Otherwise, aren't we asking people to do this? > > > > var Q = {}; > > Components.utils.import("resource:///.../promise.jsm", Q); > > So, what would you suggest? In the context of a jsm, export a symbol Q > containing function |defer|? I think that would be a good idea. > > ::: toolkit/content/promise.jsm > > @@ +16,5 @@ > > > + this.EXPORTED_SYMBOLS = Object.keys(this); > > > + } else { > > > + factory(undefined, (this.promise = {})); > > > + } > > > +})(function(require, exports) { > > > > I've a couple of modules that I support across JSM/browser via require/node, > > and I personally revolt more at the block above than the need to make smart > > updates to keep versions in sync. (That could be because my tools make this > > easy though). > > This isn't a change requirement, just a 'please consider'. > > I am willing to consider anything :) However, I am not sure I understand > what you suggest here. I *was* suggesting just having the JSM part. My process for keeping the various versions of old-promise in sync was to diff them and copy the changes across. The meant I could retain different headers/footers (also I liked the quick self review process). However dooming everyone to my process seems a bit brutal. Unless this idea is a revelation to you, I suggest you do nothing. > > @@ +78,5 @@ > > > + // something amiss. Perhaps we will want to log this, or perhaps we will > > > + // want to launch a setTimeout to refine this claim. > > > +}; > > > + > > > +function isPromise(value) { > > > > Is it worth having a utility for this? It's not exported (maybe it should > > be?) > > I believe that using this utility function in the implementation is more > readable and more maintainable than introducing an immediate check for a > method |then|. But yes, good point, let's export this. Also there's a comparable Q.isPromise() > > @@ +82,5 @@ > > > +function isPromise(value) { > > > + /** > > > + Returns true if given `value` is promise. Value is assumed to be promise if > > > + it implements `then` method. > > > + **/ > > > > I very much like the idea of python style inline comments, but one of the > > points of doc comments is uniformity across a codebase, so it's a bit > > strange having some modules being completely different like this. > > I'm not sure I care enough to argue, but it's not what I would do. > > I attempted to stick with what looks like Jetpack style (although I have not > checked whether it is normalized). If you want, just say so and I will put > it in javadoc/jsdoc style. I'd like us to share code with Jetpack. If there is a chance of us doing that then we should keep the Jetpack style. If Irakli doesn't see any way of sharing then I'm tempted to convert to a more Mozilla style. However, either way, let's not do it now. ... > > @@ +248,5 @@ > > > + var concat = Array.prototype.concat; > > > + > > > + // Utility function that does following: > > > + // execute([ f, self, args...]) => f.apply(self, args) > > > + function execute(args) { return call.apply(call, args); } > > > > Do we have evidence of an actual performance increase by doing this? > > Let me suggest also postponing |promised| to a followup bug. Huh - I missed that promised isn't in real-Q. We should definitely do that.
Attachment #630184 -
Attachment is obsolete: true
Attachment #630184 -
Flags: review?(jwalker)
Attachment #633498 -
Flags: review?(jwalker)
Here we go. I have added documentation, removed three lines of dead code that I had forgotten, and removed the features that we postponed to a followup bug.
Attachment #630183 -
Attachment is obsolete: true
Attachment #630183 -
Flags: review?(jwalker)
Attachment #633499 -
Flags: review?(jwalker)
Reporter | ||
Updated•12 years ago
|
Attachment #633499 -
Flags: review?(jwalker) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #633498 -
Flags: review?(jwalker) → review+
Assignee | ||
Comment 55•12 years ago
|
||
(In reply to Joe Walker from comment #50) > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #49) > > Comment on attachment 630183 [details] [diff] [review] > > A new implementation of promises with an API compatible with that of Jetpack > > > > Review of attachment 630183 [details] [diff] [review]: > > ----------------------------------------------------------------- > > ... > > > +function defer(prototype) { > > > > I've talked with Kris Kowal about this feature over IRC & he was going to > > add it to Q. > > OK, thanks. That seems to be a potential future solution to one point, the > other being about general utility - if there was a large clamour for this, > wouldn't Q have it already? > Also, we can add easily - it's a 10 line patch that we could all write in > our sleep, but we can't remove it nearly as easily, or maybe at all. > I vote we create a bug that asks for this to be added and attach a patch to > it and maybe add a comment to the effect, and take a look at how much it's > needed, and how Q gets on with it. > > > @@ +124,5 @@ > > > + var foo = defer(prototype) > > > + deferred.promise.get('name').then(console.log) > > > + deferred.resolve({ name: 'Foo' }) > > > + //=> 'Foo' > > > + */ > > > > Most examples and docs actually live here: > > https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/ > > promise.md > > Is this the official home of addon-sdk docs? > No official one is here https://addons.mozilla.org/en-US/developers/docs/sdk/latest/packages/api-utils/promise.html > > (Shouldn't the home be somewhat > closer to MDN?) Anyway, I agree with the notion that we should probably link > to the docs from the header. > > > @@ +248,5 @@ > > > + var concat = Array.prototype.concat; > > > + > > > + // Utility function that does following: > > > + // execute([ f, self, args...]) => f.apply(self, args) > > > + function execute(args) { return call.apply(call, args); } > > > > > Do we have evidence of an actual performance increase by doing this? > > > > I did bunch of profiling in web inspector (unfortunately jsperf was down for > > a couple of days), but we could probably do create jsperf test for it. But > > even without tests it's pretty clear that not creating closure on each call > > at line 283 makes a big diff. > > So if we're sure that it makes a difference, let's move them as close > together as we can and kill call and concat. > Personally I only sacrifice readability for performance with actual numbers > relevant to the domain of use, but this is only one line so it's really not > worth arguing about. > > Thanks Irakli.
Assignee | ||
Comment 56•12 years ago
|
||
> > So, what would you suggest? In the context of a jsm, export a symbol Q
> > containing function |defer|?
>
> I think that would be a good idea.
>
I personally would not like that:
1. In sdk modules we always provide exports without any containers.
2. Even with Q if you do `require('q')` you get exports not a `Q`.
3. This is not a `Q` it is a subset and claiming `Q` name is not good IMO.
Assignee | ||
Comment 57•12 years ago
|
||
> > Is it worth having a utility for this? It's not exported (maybe it should > > be?) > > I believe that using this utility function in the implementation is more readable > and more maintainable than introducing an immediate check for a method |then|. But > yes, good point, let's export this. > `isPromise` should not be exported and users should not test values if they are promises. If you're unsure that value is a promise then: resolve(value).then(... That is a whole point. `isPromise` was intentionally not exported to make sure you want test values for being promises.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #57) > > > Is it worth having a utility for this? It's not exported (maybe it should > > > be?) > > > > I believe that using this utility function in the implementation is more readable > > and more maintainable than introducing an immediate check for a method |then|. But > yes, good point, let's export this. > > > > `isPromise` should not be exported and users should not test values if they > are promises. If you're unsure that value is a promise then: > > resolve(value).then(... > > That is a whole point. `isPromise` was intentionally not exported to make > sure you want test values for being promises. I assume that there is a good reason for this, but performance-wise, this is a killer.
Assignee | ||
Comment 59•12 years ago
|
||
> I'd like us to share code with Jetpack. If there is a chance of us doing that
> then we should keep the Jetpack style. If Irakli doesn't see any way of sharing
> then I'm tempted to convert to a more Mozilla style.
> However, either way, let's not do it now.
Sharing code was my intention as well when I started, but it really depends what we'll end up with.
So far my implementation was forked and some idiomatic design decisions (like not exposing isPromise) have being changed. Also code was transformed IMO less maintainable style.
Also some features we use have being removed, although on SDK side we can split promise implementation into two to expose those features as separate module.
Assignee | ||
Comment 60•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #58) > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #57) > > > > Is it worth having a utility for this? It's not exported (maybe it should > > > > be?) > > > > > > I believe that using this utility function in the implementation is more readable > > > and more maintainable than introducing an immediate check for a method |then|. But > yes, good point, let's export this. > > > > > > > `isPromise` should not be exported and users should not test values if they > > are promises. If you're unsure that value is a promise then: > > > > resolve(value).then(... > > > > That is a whole point. `isPromise` was intentionally not exported to make > > sure you want test values for being promises. > > I assume that there is a good reason for this, but performance-wise, this is > a killer. 1. Please measure first and than discuss performance. 2. Here and in other places you made sacrifices code maintainability for the sake of potential performance issues, which I can't justify without actual issues. 3. If performance is that critical in some code paths I'd even go ahead and use callbacks, promises will be slower anyway. 4. Making sure that module is used properly is more important for jetpack, exposing function like this means it will be missuses. 5. In performance critical code where you really want `isPromise` just create one there.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #60) > 1. Please measure first and than discuss performance. > 2. Here and in other places you made sacrifices code maintainability for the > sake > of potential performance issues, which I can't justify without actual issues. > 3. If performance is that critical in some code paths I'd even go ahead and > use callbacks, promises will be slower anyway. > 4. Making sure that module is used properly is more important for jetpack, > exposing > function like this means it will be missuses. > 5. In performance critical code where you really want `isPromise` just > create one there. Let's not escalate this any further. I have the impression that the feature is: 1/ useful; 2/ trivial to implement from within the module; 3/ impossible to implement safely from without. If you think it useless, we can certainly remove it and/or postpone it to a followup bug. Do you see any reason to not include this feature in the module?
Assignee | ||
Comment 62•12 years ago
|
||
Yes as said above, this is feature exposes high risk of misunderstanding promises, just because it looks useful. There for it should not be part of public API.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #62) > Yes as said above, this is feature exposes high risk of misunderstanding > promises, > just because it looks useful. There for it should not be part of public API. Unless by "misunderstanding promises" you mean "using that function", I am not sure I follow what you mean.
Assignee | ||
Comment 64•12 years ago
|
||
I mean that's a foot gun. If your code branches on `isPromise(value)` you're doing it wrong, with a few exceptions where you intentionally do it for the sake of perf for example.
I would be interested in an example to back that claim.
Reporter | ||
Comment 66•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #65) > I would be interested in an example to back that claim. I think Irakli's point is that that's not the way you're supposed to use the API. On the one hand this isn't in the jetpack promise, so there's an interop reason to take it out. However, the point of isPromise(), however is interop; I've got this thing from someone, I don't know if it's a promise or not, should I call then() on it? Now jetpack is a more constrained environment, so the chance of writing jetpack code where someone gives you something that may or may not be a promise is much lower. In all my use of promises, whenever this has arrived, I've just done (typeof thing.then === 'function') before calling then(). Which would work here, and in many ways is preferable. My original implementation of promises had isPromise, but I didn't use it because (typeof thing.then === 'function') provided better interoperability. In summary, I'm leaning towards not exporting it.
(In reply to Joe Walker from comment #66) > In summary, I'm leaning towards not exporting it. I was about to suggest the same thing for different reasons. As long as we do not have a use scenario, let's not export it.
Assignee | ||
Comment 68•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #65) > I would be interested in an example to back that claim. My point is that promises imply paradigm of asynchronous behavior that is very different from synchronous one. Any attempt to of encapsulating these two paradigms into a component will result into asynchronous one or encapsulation will leak. Properly encapsulating these two paradigms is not easy, having many places where such encapsulation happens increases error surface area. With a jetpack promise API this surface is reduced to a single place. Now in practice only thing you can do with `isPromise` function is to branch logic depending on weather you already have a value or not: if (isPromise(value) { doSomething(value) } else { value.then(doSomething) } Because of this branching behavior may differ on input type not done carefully. This can be avoided all together: resolve(value).then(doSomething) All the branching happens with in the library and makes it impossible to have a behavior differences no matter if given input is eventual or not.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #68) > (In reply to David Rajchenbach Teller [:Yoric] from comment #65) > > I would be interested in an example to back that claim. > > My point is that promises imply paradigm of asynchronous behavior that is > very different from synchronous one. Any attempt to of encapsulating these > two paradigms into a component will result into asynchronous one or > encapsulation will leak. > [...] > All the branching happens with in the library and makes it impossible to > have a behavior differences no matter if given input is eventual or not. Interestingly, this is the case of scenario that I already need to cope with some existing synchronous code and refactor it into partially asynchronous code. However, this does not change my previous stance: for the moment, let's not include isPromise in the API.
Attachment #633499 -
Attachment is obsolete: true
Assignee | ||
Comment 71•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•12 years ago
|
Attachment #633711 -
Flags: superreview?(dtownsend+bugmail)
Attachment #633711 -
Flags: review?(jwalker)
Attachment #633711 -
Flags: review?(dteller)
Assignee | ||
Comment 72•12 years ago
|
||
So I'm submitting this updated patch of the SDK promise library that: 1. Adds semicolons. 2. Removes some features that have being removed in forked version. 3. Changes resolution code to use while (index ++) instead of disliked while(items.lenght) items.shift() I'm submitting this as I'm convinced that :Yoric's fork is not simpler, but quite an opposite. Only way to know that is to get more people looking into this.
Comment on attachment 633711 [details] Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/4 Irakli, this is an improvement on your previous version. I would have appreciated if you could have made such improvements two months ago when I asked for it, instead of refusing to do so and waiting until I had rewritten and documented everything three times. Now, I personally think that my latest versions are much more compliant to mozilla-central guidelines, and in particular much more approachable by potential contributors.
Attachment #633711 -
Flags: review?(dteller) → feedback-
Updated•12 years ago
|
Attachment #633649 -
Flags: review+
Reporter | ||
Comment 74•12 years ago
|
||
So the problem is a disagreement over what it means to be simple. I don't think we make good enough progress swapping patches. We could solve this by stepping up a level and discussing abstractly quantities such as brevity and triviality, levels of documentation, code idioms, and ranking them to agree as order of importance, before then stepping back down to look at the code. However, above all of that; value forward momentum. When the cost of getting the right answer is greater than the value of the right answer; accept second best, and move on. Lets get this in, and improve it in follow-up bugs if needs be.
(In reply to Joe Walker from comment #74) > Lets get this in, and improve it in follow-up bugs if needs be. Fine with me.
Assignee | ||
Comment 76•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #73) > Comment on attachment 633711 [details] > Pointer to Github pull request: > https://github.com/mozilla/mozilla-central/pull/4 > > Irakli, this is an improvement on your previous version. I would have > appreciated if you could have made such improvements two months ago when I > asked for it, instead of refusing to do so and waiting until I had rewritten > and documented everything three times. > As I mentioned back then I was busy with Jetpack Q2 goals and this had a lower priority. Although I remember pointing out that addressing your concerns did not required rewrites, which you have decided to do anyway.
Assignee | ||
Comment 77•12 years ago
|
||
Comment on attachment 633649 [details] [diff] [review] A new implementation of promises with an API compatible with that of Jetpack Review of attachment 633649 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/promise.jsm @@ +79,5 @@ > + if (!onResolve) { > + deferred.resolve(value); > + return; > + } > + box(onResolve, value, deferred); It would be much easier to follow logic if you would not invert logic: if (onReslove) box(onResolve, value, deferred); else deferred.resolve(value); @@ +83,5 @@ > + box(onResolve, value, deferred); > +} > + > +// Utility function to execute a `onReject` handler, if it is defined > +function executeOnReject(onReject, value, deferred) { Same as above. @@ +94,5 @@ > + > +// Utility function: execute |f(value)|, transmit > +// success to |deferred.resolve| or failure to > +// |deferred.reject|. > +function box(f, value, deferred) { Why `box` ? maybe `forward` or something ? @@ +95,5 @@ > +// Utility function: execute |f(value)|, transmit > +// success to |deferred.resolve| or failure to > +// |deferred.reject|. > +function box(f, value, deferred) { > + var result; Why writing such a confusing style ? Following would make it way more logical. try { deferred.reslove(f(value)) } catch (error) { deferred.reject(error) } @@ +113,5 @@ > +// all observers. > +// Argument `inLock` is used to lock a promise to ensure that only the > +// first `resolve`/`reject` is taken into account, even if it is > +// itself implemented as a (possibly asynchronous) promise. > +var propagate = function propagate(inLock, state, isResolve, value) { This DSL like function is hard to follow. @@ +114,5 @@ > +// Argument `inLock` is used to lock a promise to ensure that only the > +// first `resolve`/`reject` is taken into account, even if it is > +// itself implemented as a (possibly asynchronous) promise. > +var propagate = function propagate(inLock, state, isResolve, value) { > + if (!inLock && state.status != undefined) { What is even point of this argument ? Why would you even allow calling this multiple times ? If promise is resolved it should be it no matter if it's resolved with a value or promise. Keeping track of when promise is locked and when not is almost impossible. @@ +167,5 @@ > + }; > + > + var then = function then(onResolve, onReject) { > + var deferred = defer(); > + if (state.observers) /*result is not known yet*/{ So you do have `state.status` for that no ? Combination of two fields is very confusing ? @@ +168,5 @@ > + > + var then = function then(onResolve, onReject) { > + var deferred = defer(); > + if (state.observers) /*result is not known yet*/{ > + state.observers.push([onResolve, onReject, deferred]); observes array implies that it's collection of observers not observers and deferreds. Also I was pretty much convinced that you never resolve `deferred` here. It is not obvious that you do at least some comment would help. @@ +169,5 @@ > + var then = function then(onResolve, onReject) { > + var deferred = defer(); > + if (state.observers) /*result is not known yet*/{ > + state.observers.push([onResolve, onReject, deferred]); > + } else if (state.status) /*resolution*/ { So there are: `state.status`, `state.resolved`, `state.observers` all these to indicate the state the promise is in. Could you simplify ? Otherwise I need to write down a matrix to look into to figure out the state ? @@ +178,5 @@ > + return deferred.promise; > + }; > + > + var resolve = function resolve(value) { > + if (!isPromise(value)) { should not you mark state as resolved here ? @@ +185,5 @@ > + } else { > + // If |value| is a promise, things get a bit more complex: > + // - lock state to ensure that no further |resolve|/|reject| is > + // accepted; > + state.resolved = true; You define all the state properties at creation, why is it exception ? Either please indicate a reason or be consistent. @@ +189,5 @@ > + state.resolved = true; > + // - propagate result once |value.then| is resolved/rejected; > + // - ... unless it is again a promise, in which case we need > + // to do this recursively. > + var resolve2 = function resolve2(value) { resolve2 is very confusing name. Also function reslove2(value) { ... } is enough no need for a `var`. @@ +196,5 @@ > + } else { > + value.then(resolve2, reject2); > + } > + }; > + var reject2 = function reject2(value) { reject2 is again very confusing name @@ +203,5 @@ > + value.then(resolve2, reject2); > + } > + }; > + var reject = function reject(value) { > + // FIXME: In a future version, we may want to handle the case reject should be able to handle promises otherwise some scenarios of value / error propagation will not work.
Attachment #633649 -
Flags: feedback-
Thanks for the review, I will try and address your remarks by Tuesday.
Assignee | ||
Comment 79•12 years ago
|
||
I'm quite surprised how we end up here. We all greed on API and semantics and plan was to request a super review from Mossop. Then I got some feedback on function naming and questionable "complexity", addressing which was matter of few line changes, but for still unclear reasons, forked, completely rewritten version got submitted for a review. I think it's really unproductive attitude. In addition: 1. I do really think that forked implementation is unnecessary complicated, with dreaded logic that is hard to follow and reason about. I really can't tell if there are some flows still left or not (in all previous drafts there were some hidden bugs), as it got too complicated and I don't have enough registers in my brain to picture a state each individual scenario. Specially `propagate` function, it's a DSL that is hard to keep track of. 2. The only justification of fork was the claim that original implementation was too complicated. Now fork appears to be even more complicated, more code, more logic branches, more places modifying state. I don't know about new comers, that supposedly will have a better time contributing to this code, but I can tell for myself that unreasonable complexity makes it very difficult for me, and I consider myself to be an experienced engineer. Now we're talking about landing this version and make an improvements to it later. My question is how does this make any sense: we did not wanted to check in original version because of questionable complexity, but now it's ok to submit way more complicated version ? This whole situation puts me in a really awkward position. 3. I get re-concurring comments about poor code quality (sometimes inappropriate ones) of the original implementation and how not compliant it is with Mozilla standards. These comments are not only unfair but also offensive to everyone involved in the jetpack project. Code checked into SDK repository (including a promise lib), is verified to be compliant with mozilla code quality by mozilla reviewers. Promise lib shipped with an SDK went to standard review process, has tests verifying implementation quality and in addition has being battle tested in real life.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #77) > Comment on attachment 633649 [details] [diff] [review] > A new implementation of promises with an API compatible with that of Jetpack > > Review of attachment 633649 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/promise.jsm > @@ +79,5 @@ > > + if (!onResolve) { > > + deferred.resolve(value); > > + return; > > + } > > + box(onResolve, value, deferred); > > It would be much easier to follow logic if you would not invert logic: > > if (onReslove) > box(onResolve, value, deferred); > else > deferred.resolve(value); Ok. > @@ +83,5 @@ > > + box(onResolve, value, deferred); > > +} > > + > > +// Utility function to execute a `onReject` handler, if it is defined > > +function executeOnReject(onReject, value, deferred) { > > Same as above. Ok. > @@ +94,5 @@ > > + > > +// Utility function: execute |f(value)|, transmit > > +// success to |deferred.resolve| or failure to > > +// |deferred.reject|. > > +function box(f, value, deferred) { > > Why `box` ? maybe `forward` or something ? I agree that |box| is not a very nice name. Here, I lack inspiration for a good name. Indeed, |forward| might be a little better. > @@ +95,5 @@ > > +// Utility function: execute |f(value)|, transmit > > +// success to |deferred.resolve| or failure to > > +// |deferred.reject|. > > +function box(f, value, deferred) { > > + var result; > > Why writing such a confusing style ? Following would > make it way more logical. > > try { > deferred.reslove(f(value)) > } catch (error) { > deferred.reject(error) > } Haven't we had this discussion before? Your snippet is tighter but has the wrong semantics: we do not want to catch errors in |deferred.resolve|. > @@ +113,5 @@ > > +// all observers. > > +// Argument `inLock` is used to lock a promise to ensure that only the > > +// first `resolve`/`reject` is taken into account, even if it is > > +// itself implemented as a (possibly asynchronous) promise. > > +var propagate = function propagate(inLock, state, isResolve, value) { > > This DSL like function is hard to follow. [...] > What is even point of this argument ? Why would you even allow calling this > multiple times ? If promise is resolved it should be it no matter if it's > resolved with a value or promise. Keeping track of when promise is locked > and when not is almost impossible. You are right. > @@ +167,5 @@ > > + }; > > + > > + var then = function then(onResolve, onReject) { > > + var deferred = defer(); > > + if (state.observers) /*result is not known yet*/{ > > So you do have `state.status` for that no ? Combination of two fields is > very confusing ? Good point. > > @@ +168,5 @@ > > + > > + var then = function then(onResolve, onReject) { > > + var deferred = defer(); > > + if (state.observers) /*result is not known yet*/{ > > + state.observers.push([onResolve, onReject, deferred]); > > observes array implies that it's collection of observers not > observers and deferreds. Also I was pretty much convinced that > you never resolve `deferred` here. It is not obvious that you > do at least some comment would help. Adding some comments. I rather like name |observers|, but if you have a better name in mind, I am willing to change it. > > @@ +169,5 @@ > > + var then = function then(onResolve, onReject) { > > + var deferred = defer(); > > + if (state.observers) /*result is not known yet*/{ > > + state.observers.push([onResolve, onReject, deferred]); > > + } else if (state.status) /*resolution*/ { > > So there are: `state.status`, `state.resolved`, `state.observers` all these > to indicate the state the promise is in. Could you simplify ? Otherwise I > need to write down a matrix to look into to figure out the state ? Well, indeed, strangely, the state is defined by variable state :) More seriously, you are right that I can make this a little clearer by only branching on |state.status|. > > @@ +178,5 @@ > > + return deferred.promise; > > + }; > > + > > + var resolve = function resolve(value) { > > + if (!isPromise(value)) { > > should not you mark state as resolved here ? You are right, this would be clearer. > @@ +185,5 @@ > > + } else { > > + // If |value| is a promise, things get a bit more complex: > > + // - lock state to ensure that no further |resolve|/|reject| is > > + // accepted; > > + state.resolved = true; > > You define all the state properties at creation, why is it exception ? > Either please indicate a reason or be consistent. I do not understand that comment. > > @@ +189,5 @@ > > + state.resolved = true; > > + // - propagate result once |value.then| is resolved/rejected; > > + // - ... unless it is again a promise, in which case we need > > + // to do this recursively. > > + var resolve2 = function resolve2(value) { > > resolve2 is very confusing name. Also function reslove2(value) { ... } is > enough no need for a `var`. Fixed that. > > @@ +196,5 @@ > > + } else { > > + value.then(resolve2, reject2); > > + } > > + }; > > + var reject2 = function reject2(value) { > > reject2 is again very confusing name Fixed that, too. > @@ +203,5 @@ > > + value.then(resolve2, reject2); > > + } > > + }; > > + var reject = function reject(value) { > > + // FIXME: In a future version, we may want to handle the case > > reject should be able to handle promises otherwise some scenarios of value / > error propagation will not work. Fair enough.
Applied irakli's feedback.
Attachment #633649 -
Attachment is obsolete: true
Attachment #633896 -
Flags: review?(jwalker)
Attachment #633896 -
Flags: feedback?(rFobic)
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #79) > I'm quite surprised how we end up here. We all greed on API and semantics > and plan > was to request a super review from Mossop. Then I got some feedback on > function naming > and questionable "complexity", addressing which was matter of few line > changes, but for still unclear reasons, forked, completely rewritten version > got submitted for a review. I think it's really unproductive attitude. Well, I believe that the reason was rather simple. I submitted a patch some time ago which addressed the issues of function/variable/argument naming, as well as added some documentation that was sorely needed, and you never reviewed that patch. Since there were other issues that complicated the code – admittedly less clean-cut than the issues of documentation and naming – and since you have made it repeatedly rather clear that you would not change anything, the only solution was to finish a revised version and submit it, so that it could be compared. Now, I am happy to see that either you have changed your mind or this was a communication issue and that you are indeed willing to change things. As mentioned before, this is a bit late, but still welcome. > > In addition: > > 1. I do really think that forked implementation is unnecessary complicated, > with dreaded logic that is hard to follow and reason about. I really can't > tell if there > are some flows still left or not (in all previous drafts there were some > hidden bugs), > as it got too complicated and I don't have enough registers in my brain to > picture a state each individual scenario. Specially `propagate` function, > it's a DSL that is hard to keep track of. I agree that it is not nice enough yet. It started simple, but as I understood more subtleties of the semantics of your implementation (the "hidden bugs" you mention), it has grown to something that I do not like overmuch. In the latest version, I believe that I have managed to simplify it. Could you please tell me what you think of this version? > > 2. The only justification of fork was the claim that original implementation > was > too complicated. Now fork appears to be even more complicated, more code, > more > logic branches, more places modifying state. I don't know about new comers, > that supposedly will have a better time contributing to this code, but I can > tell for > myself that unreasonable complexity makes it very difficult for me, and I > consider > myself to be an experienced engineer. Now we're talking about landing this > version > and make an improvements to it later. My question is how does this make any > sense: > we did not wanted to check in original version because of questionable > complexity, > but now it's ok to submit way more complicated version ? This whole > situation puts > me in a really awkward position. I grant you that we can still improve the forked version – thanks for pointing out a few possible improvements, by the way. However, the main difference is that this forked version is much more explicit than the unforked version. The unforked version is full of elegant and tightly-packed subtleties that unfortunately make it very easy to misunderstand the code or to give up reading it and very hard to make the slightest change without breaking everything. By contrast, the forked version, while less elegant, is much easier to patch and, I believe, to read. This patch-ability makes it easier to land improvements progressively. > > 3. I get re-concurring comments about poor code quality (sometimes > inappropriate ones) of the original implementation and how not compliant it > is > with Mozilla standards. These comments are not only unfair but also > offensive to > everyone involved in the jetpack project. Code checked into SDK repository > (including > a promise lib), is verified to be compliant with mozilla code quality by > mozilla reviewers. Promise lib shipped with an SDK went to standard review > process, has tests verifying implementation quality and in addition has > being battle tested in real life. Since both parties have offered and accepted apologies for their respective inappropriate/unfair/offensive comments, I will not continue on this topic. Rather, I would like to point out something was I suspect was lost in the conversation. I do not believe I ever said that code was not compliant with _Mozilla_ standards/quality/guidelines/... (or, if I have, I apologize, as this was not what I meant), but with _mozilla-central_ standards/quality/guidelines. This is very different. While I am by no mean an expert on the Jetpack review process, it seems clear that this process is very different, and has different aims, than any review I have seen in mozilla-central, and in particular in toolkit/. Every suggestion/piece of documentation/patch that I have provided during this long process was meant to ensure that the piece of code that eventually lands in toolkit respects the guidelines of toolkit.
Reporter | ||
Updated•12 years ago
|
Attachment #633896 -
Flags: review?(jwalker) → review+
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #56) > > > So, what would you suggest? In the context of a jsm, export a symbol Q > > > containing function |defer|? > > > > I think that would be a good idea. > > > > I personally would not like that: > > 1. In sdk modules we always provide exports without any containers. > 2. Even with Q if you do `require('q')` you get exports not a `Q`. > 3. This is not a `Q` it is a subset and claiming `Q` name is not good IMO. On second thought, I agree with Irakli that calling this |Q| is probably premature. Perhaps |Promise|?
Reporter | ||
Comment 84•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #83) > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #56) > > > > So, what would you suggest? In the context of a jsm, export a symbol Q > > > > containing function |defer|? > > > > > > I think that would be a good idea. > > > > > > > I personally would not like that: > > > > 1. In sdk modules we always provide exports without any containers. > > 2. Even with Q if you do `require('q')` you get exports not a `Q`. > > 3. This is not a `Q` it is a subset and claiming `Q` name is not good IMO. > > On second thought, I agree with Irakli that calling this |Q| is probably > premature. Perhaps |Promise|? That it's a subset of Q and not Q isn't an issue to me - the worst that would happen is that someone would try to use a method that wasn't there. The route to correction would be short. The point of the subset-of-Q idea is: - Q is the closest to a 'standard' we have - Using as little of it as we can is good because it's not yet a standard I am swayed however that we should provide exports without any containers. Sorry Yoric, I think that one was my fault. (Side note I'm at QCon this week. Expect slow responses. If you can agree between you, then you don't need r+ from me.)
Assignee | ||
Comment 85•12 years ago
|
||
First of all, I'm glad we're getting to a more constructive face now :) > > Well, I believe that the reason was rather simple. I submitted a patch some > time ago which addressed the issues of function/variable/argument naming, as > well as added some documentation that was sorely needed, and you never > reviewed that patch. > That is unfortunate, I never saw such patch. Starting form very first one, and all the following ones were complete rewrites (At first we could not even agree on semantics, as far as I recall). Either way it's not important now, lets just move on. > > The unforked version is full of elegant and tightly-packed > subtleties that unfortunately make it very easy to misunderstand the code or > to give up reading it and very hard to make the slightest change without > breaking everything. > > By contrast, the forked version, while less elegant, is much easier to patch and, > I believe, to read. This patch-ability makes it easier to land improvements > progressively. I'm happy you find solutions elegant, although I don't quite agree with other points you make. Either way I'll suggest to go ahead with version that already proved itself in the SDK. If requirement for changes and improvements will appear we can discuss the best way to make them (Maybe even rewrite). Till then, it's just a waste of time. As of your comments in pull request, I plan to address them as a first thing on Monday morning.
Well, requirements for changes and improvements appeared back in #comment 27, with the ability to log suspicious rejections to help with debugging. I suspect other changes will creep up on us without warning. I'd suggest the following: address the comments I have left for you on pull-request, then we will both take a break from this and let someone else read our codes and decide.
Assignee | ||
Comment 87•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #86) > Well, requirements for changes and improvements appeared back in #comment > 27 That is a one line change and I'd be happy to submit a patch for it. > > with the ability to log suspicious rejections to help with debugging. > I suspect other changes will creep up on us without warning. > There is multiple ways to address "suspicious rejections" logging without changes to a promise implementation itself. I'm more than happy to iterate over this and other issues, but please lets defer that until it's relevant.
Assignee | ||
Comment 88•12 years ago
|
||
Update on the current progress: I have got tons of comments from Yoric in pull request and I believe I have addressed
Assignee | ||
Comment 89•12 years ago
|
||
Oops I have submitted changes little earlier than desired. Another try: Update on the current progress: I have got tons of comments from Yoric in pull request and I believe I have addressed all of them except few: 1. I have decided to defer cuddling else / catch style issues since that were Mozilla central and SDK style guides seem to have incompatible recommendations. We can come back to this once that's only thing left. 2. I insist that FIFO order of handler invocation must be guaranteed for several reasons: - Q that we claim to be a subset of has that, such a behavioral differences would be hard to spot and workaround. - Asynchronous computations expressed via promises will have FIFO order, this implies some expectations. Invalidating those expectations when mixing with synchronous computations would make reasoning about code harder and switches from sync to async or wise versa problematic. - There are particular cases [described earlier](https://github.com/mozilla/mozilla-central/pull/4#discussion_r1004760) where no guarantees about order that makes proper solutions a lot more complicated. Also wrong assumptions about order won't be apparent to users and may become very tricky to spot. - Expectations about order is already set by an environment JS runs. Event listeners etc all have deterministic FIFO order new semantics will make sings only harder. 3. I tend to disagree that dequeue until exhausted pattern is confusing or harder to follow than nested resetting loops. There for I would like to get some other feedback to find out. 4. I got comments about observer pairs and it was not very clear if should have added comments or change. At the moment I have chose to documented why current approach is used instead of fork - join, although I'm open to change if that would be an improvement. Yoric please let me know (in pull request, since this is just update for causal followers) if I miss something. Thanks!
Assignee | ||
Comment 90•12 years ago
|
||
Comment on attachment 633896 [details] [diff] [review] A new implementation of promises with an API compatible with that of Jetpack v4 Review of attachment 633896 [details] [diff] [review]: ----------------------------------------------------------------- While I think it's much better I still: - Dislike that many logic branches (8 vs 2), it makes unnecessary more complicated to follow the code, as one needs to mentally move back and forth in the call stack to see cover the whole flow. - For reasons pointed out in bugzilla I think FIFO order is really important which is not guaranteed by this implementation. - Invalid promises may infect whole chain of promises causing inconsistencies. This may sound like an unlike situation, but it's more problematic than that. When interacting with possible untrusted / malicious code this may be used intentionally used to cause surprises. Since SDK interacts with untasted code I consider this important. ::: toolkit/content/promise.jsm @@ +65,5 @@ > + } else if (typeof(exports) === 'object') { // CommonJS > + factory(require, exports); > + } else if (String(this).indexOf('BackstagePass') != -1) { // JSM > + factory(undefined, (this.Q = {})); > + this.EXPORTED_SYMBOLS = ["Q"]; As I already pointed out I think it's wrong on many levels to claim name `Q` of the other library. I also don't see any value in doing it. @@ +73,5 @@ > +})(function(require, exports) { > + > +'use strict'; > + > +// Utility function to execute a `onResolve` handler, if it is defined That is very confusing description of what this function does. I think you should at least note what if it is not defined. Also it's not very obvious that `forwardToDeferred` will resolve `deferred` with a `onResolve(value)`. Maybe comment or some other name could help that. I would probably have change this to something like this: // Utility function that resolves `deferred` with either `f(value)` or `value` depending on weather `f` is defined or not. function resloveWith(deferred, value, f) { ... @@ +76,5 @@ > + > +// Utility function to execute a `onResolve` handler, if it is defined > +function executeOnResolve(onResolve, value, deferred) { > + if (onResolve) { > + forwardToDeferred(onResolve, value, deferred); Nit: I tend to prefer see function definitions earlier than their use. Having `forwardToDeferred` definition earlier would impreve readability IMO. @@ +82,5 @@ > + deferred.resolve(value); > + } > +} > + > +// Utility function to execute a `onReject` handler, if it is defined Same as above, this comment does not actually explains what the function does. @@ +94,5 @@ > + > +// Utility function: execute |f(value)|, transmit > +// success to |deferred.resolve| or failure to > +// |deferred.reject|. > +function forwardToDeferred(f, value, deferred) { Maybe changing name to something like `resolveAs` or `reslove*` would make it more clear what function does. Also in that case I would have made `deferred` a first argument, rather than last. @@ +98,5 @@ > +function forwardToDeferred(f, value, deferred) { > + var result; > + try { > + result = f(value); > + } catch (x) { I think `error` or `e` is a better name than `x`. @@ +103,5 @@ > + deferred.reject(x); > + return; > + } > + // We separate this call from the try/catch to ensure that we do > + // not capture unintended errors (such as internal errors). So we just ignore them ? That is what I meant by reducing surface for bugs, by having less logic branches. @@ +115,5 @@ > + var observers = state.observers; > + state.observers = null; > + state.result = value; > + state.status = isResolve; > + for (var i = 0; i < observers.length; ++i) { Probably it's clear by now but this does not guarantees FIFO order of observer delegation. I have outlined issues associated with that in bugzilla. @@ +119,5 @@ > + for (var i = 0; i < observers.length; ++i) { > + var obs = observers[i]; > + var onResolve = obs[0]; > + var onReject = obs[1]; > + var deferred = obs[2]; I still think that it's awkward to have `deferred` as part of `obs` which I interpret as `observer` @@ +168,5 @@ > + result: undefined, > + /** > + * |undefined| until we know whether it is resolved or rejected > + * |true| if it is resolved > + * |false| if it is rejected While reading code I honestly wished you have provided constants like PENDING, RESOLVED, REJECTED. Even though I don't generally like that it was little confusing to think of boolean with three possible values. @@ +177,5 @@ > + var then = function then(onResolve, onReject) { > + var deferred = defer(); > + if (state.status == undefined) /*result is not known yet*/{ > + // `deferred` will be resolved later > + state.observers.push([onResolve, onReject, deferred]); I find it awkward that `deferred` is part of observers. @@ +198,5 @@ > + }; > + var propagateRejection = function propagateRejection(value) { > + propagate(state, false, value); > + }; > + var resolve = function resolve(value) { So, it's either you miss something here or it's just hard for me to follow. In particular case with `value` promise is confusing. I'm under impression that if `value` is invalid promise (one that misbehaves, for example calls handler more than once or calls both of them) it could cause behavior where promise may appear resolved to one value for some consumers and resolved as other for others. @@ +201,5 @@ > + }; > + var resolve = function resolve(value) { > + if (state.status != undefined) { > + // Drop resolution of an already fulfilled promise > + // FIXME: In debug mode, we may wish to add logging here I still think that this is bad approach. It is pretty common to let different parts of code to race to resolve promise to a first one to complete. If for some reason subsequent resolutions are wrong one should just expose alternative `resolve` which throws on subsequent attempts to resolve or even dump warning if that's preferred. If in majority of cases that's desired behavior than I'd suggest revisiting decision of using promises in first place.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #87) > (In reply to David Rajchenbach Teller [:Yoric] from comment #86) > > Well, requirements for changes and improvements appeared back in #comment > > 27 > > That is a one line change and I'd be happy to submit a patch for it. You obviously haven't understood my comment. The change I was talking about is logging. > > > > > with the ability to log suspicious rejections to help with debugging. > > I suspect other changes will creep up on us without warning. > > > > > There is multiple ways to address "suspicious rejections" logging without > changes to a promise implementation itself. I'm more than happy to iterate > over this and other issues, but please lets defer that until it's relevant. Well, it has been relevant in my use of promises for about 6 months now. We can continue deferring it, but I do not want to work with a patch that I suspect will require a major rewrite to add this feature.
By the way, since we are discussing future evolution of the promise library, I suspect that, for performance reasons, I will need at some point the ability to create promises for which I can determine if they are already resolved/rejected. This would go into another bug, of course, but let's keep this somewhere in mind.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #90) > Comment on attachment 633896 [details] [diff] [review] > A new implementation of promises with an API compatible with that of Jetpack > v4 > > Review of attachment 633896 [details] [diff] [review]: > ----------------------------------------------------------------- > > While I think it's much better I still: > > - Dislike that many logic branches (8 vs 2), it makes unnecessary more > complicated to follow the code, as one needs to mentally move back and forth > in the call stack to see cover the whole flow. I believe we just disagree on that. See my patch documenting a subset of the logics of your |then| (as it was one month ago, but the algorithm hasn't changed): https://github.com/Yoric/addon-sdk/commit/c33ced971ae34a0597da033022d4cce9053de4c7 > - For reasons pointed out in bugzilla I think FIFO order is really important > which is not guaranteed by this implementation. I replied to this on github, I will do it on bugzilla, too. > - Invalid promises may infect whole chain of promises causing > inconsistencies. This may sound like an unlike situation, but it's more > problematic than that. When interacting with possible untrusted / malicious > code this may be used intentionally used to cause surprises. Since SDK > interacts with untasted code I consider this important. Er... sure... You lost me there. What does this have to do with my patch? > > ::: toolkit/content/promise.jsm > @@ +65,5 @@ > > + } else if (typeof(exports) === 'object') { // CommonJS > > + factory(require, exports); > > + } else if (String(this).indexOf('BackstagePass') != -1) { // JSM > > + factory(undefined, (this.Q = {})); > > + this.EXPORTED_SYMBOLS = ["Q"]; > > As I already pointed out I think it's wrong on many levels to claim name `Q` > of the other library. I also don't see any value in doing it. As mentioned above, I agree with you :) > @@ +73,5 @@ > > +})(function(require, exports) { > > + > > +'use strict'; > > + > > +// Utility function to execute a `onResolve` handler, if it is defined > > That is very confusing description of what this function does. > I think you should at least note what if it is not defined. > Also it's not very obvious that `forwardToDeferred` will resolve > `deferred` with a `onResolve(value)`. Maybe comment or some other > name could help that. > > > I would probably have change this to something like this: > > // Utility function that resolves `deferred` with either > `f(value)` or `value` depending on weather `f` is defined > or not. > function resloveWith(deferred, value, f) { ... Good idea. I'll do something like that. > > @@ +76,5 @@ > > + > > +// Utility function to execute a `onResolve` handler, if it is defined > > +function executeOnResolve(onResolve, value, deferred) { > > + if (onResolve) { > > + forwardToDeferred(onResolve, value, deferred); > > Nit: I tend to prefer see function definitions earlier than their use. > Having `forwardToDeferred` definition earlier would impreve readability IMO. I believe we will have to live with this style difference. I do not enjoy reading the source code of a function before I know how it is used. > > @@ +82,5 @@ > > + deferred.resolve(value); > > + } > > +} > > + > > +// Utility function to execute a `onReject` handler, if it is defined > > Same as above, this comment does not actually explains what the function > does. Added some doc. Although we are talking about a trivial three lines function. > > @@ +94,5 @@ > > + > > +// Utility function: execute |f(value)|, transmit > > +// success to |deferred.resolve| or failure to > > +// |deferred.reject|. > > +function forwardToDeferred(f, value, deferred) { > > Maybe changing name to something like `resolveAs` or `reslove*` would make > it more clear what function does. Also in that case I would have made > `deferred` a first argument, rather than last. Done. > @@ +98,5 @@ > > +function forwardToDeferred(f, value, deferred) { > > + var result; > > + try { > > + result = f(value); > > + } catch (x) { > > I think `error` or `e` is a better name than `x`. As you wish. I think that won't change anything either way. > > @@ +103,5 @@ > > + deferred.reject(x); > > + return; > > + } > > + // We separate this call from the try/catch to ensure that we do > > + // not capture unintended errors (such as internal errors). > > So we just ignore them ? > That is what I meant by reducing surface for bugs, by having less logic > branches. We do not ignore them, we let the JavaScript handle the internal error (and presumably do some crashing) without interfering. Catching an internal implementation error, as you suggest, and treating it as a user error, is by itself an error. > @@ +115,5 @@ > > + var observers = state.observers; > > + state.observers = null; > > + state.result = value; > > + state.status = isResolve; > > + for (var i = 0; i < observers.length; ++i) { > > Probably it's clear by now but this does not guarantees FIFO order of > observer delegation. > I have outlined issues associated with that in bugzilla. Clear. > > @@ +119,5 @@ > > + for (var i = 0; i < observers.length; ++i) { > > + var obs = observers[i]; > > + var onResolve = obs[0]; > > + var onReject = obs[1]; > > + var deferred = obs[2]; > > I still think that it's awkward to have `deferred` as part of `obs` which I > interpret as `observer` Well, it does mean |observer|. So far, I have not found a better name, but if you find one, I will be interested. > > @@ +168,5 @@ > > + result: undefined, > > + /** > > + * |undefined| until we know whether it is resolved or rejected > > + * |true| if it is resolved > > + * |false| if it is rejected > > While reading code I honestly wished you have provided constants like > PENDING, RESOLVED, REJECTED. Even though I don't generally like that it was > little confusing to think of boolean with three possible values. I was pondering that. Let's do it. > > @@ +177,5 @@ > > + var then = function then(onResolve, onReject) { > > + var deferred = defer(); > > + if (state.status == undefined) /*result is not known yet*/{ > > + // `deferred` will be resolved later > > + state.observers.push([onResolve, onReject, deferred]); > > I find it awkward that `deferred` is part of observers. Well, it is the context of the observers. I do not see anything wrong with this. > > @@ +198,5 @@ > > + }; > > + var propagateRejection = function propagateRejection(value) { > > + propagate(state, false, value); > > + }; > > + var resolve = function resolve(value) { > > So, it's either you miss something here or it's just hard for me to follow. > In particular case with `value` promise is confusing. > I'm under impression that if `value` is invalid promise (one that > misbehaves, for example calls handler more than once or calls both of them) > it could cause behavior where promise may appear resolved to one value for > some consumers and resolved as other for others. You are absolutely right, I had failed to take into account the possibility of a hostile/bogus promise. I will try and fix this. In the meantime, could you please update the documentation of your code to mention this? > > @@ +201,5 @@ > > + }; > > + var resolve = function resolve(value) { > > + if (state.status != undefined) { > > + // Drop resolution of an already fulfilled promise > > + // FIXME: In debug mode, we may wish to add logging here > > I still think that this is bad approach. It is pretty common to let > different parts of code to race to resolve promise to a first one to > complete. If for some reason subsequent resolutions are wrong one should > just expose alternative `resolve` which throws on subsequent attempts to > resolve or even dump warning if that's preferred. > > If in majority of cases that's desired behavior than I'd suggest revisiting > decision of using promises in first place. As mentioned half a dozen times already, this is a debugging tool. By opposition to your scenario, I do not have the luxury of working of a cleanroom implementation. Rather, I need promises for refactoring old, complicated and sometimes buggy code, and I need all the debugging info I can get.
Attachment #633896 -
Attachment is obsolete: true
Attachment #633896 -
Flags: feedback?(rFobic)
Attachment #634351 -
Flags: feedback?(rFobic)
Comment 95•12 years ago
|
||
I apologise for being a poor module owner and not keeping up to date enough to see what was going on here. It seems you are at this point iterating over two different implementations of the same code, mostly disagreeing on subjective metrics. That's a waste of everyone's extremely valuable time. We need to pick one implementation and drive it to landing. I guess I get the fun job of being the arbiter here. Fun because having looked over the two patches both of them are in extremely good shape, were one of them to come across my review queue alone I'd happily suggest a few minor tweaks and see it landed. There is almost no advantage of one patch over the other which means I'm going to have to decide based on my own largely subjective reasons. It took me a little while to wrap my head around how both patches worked, mostly because the promises pattern is something I haven't used much, is quite complex and is extremely poorly documented in the only specs I've been able to find. Ultimately though I found Irakli's use of fewer and simpler helper functions made it easier to follow what was going on. The way resolution promise chaining works makes more sense to me than the almost recursive nature of David's patch. The larger comments that Irakli has are also great. If there are demonstrable technical concerns with Irakli's patch that I haven't seen when wading through the mass of comments in this bug and on github then now is the time to shout about them, otherwise I think we should now focus our time on making sure any final issues with Irakli's patch are addressed, get the test suite written that we'll need before any of this can land and then get it in. David, I'm sorry that this means a loss of the work you put into your implementation here but I'd like to thank both of you for the amount of time you've spent on this. Though perhaps longer than necessary I'm sure it will end up with us having better code to land than might otherwise have been the case, and for shared modules like this, the more careful we are the better.
Updated•12 years ago
|
Attachment #634351 -
Flags: feedback?(rFobic)
Thanks for taking the time to serve as referee, Dave. I believe that the exercise was not a complete waste of time. I know that I have learnt a few things along the way, and I believe that both implementations have considerably improved during the course of this process. Actually, had Irakli's implementation (including documentation) been at this level when I first met it, I would probably not have felt compelled to provide an alternative implementation. Unfortunately, I still have one concern: I still really do not have have the slightest clue of how to instrument Irakli's code to add the debugging support I have needed so far when working with hostile-to-refactoring code. If Irakli assures me that this is feasible without major refactoring to his code, I will happily let his patch land and move this point of the task to a followup bug. P.S.: I feel compelled to take some credit for Irakli's documentation, just as Irakli deserves some credit for my code.
For future reference, I upload the latest version of my code, which was pending tests.
Attachment #634351 -
Attachment is obsolete: true
(sorry, wrong patch)
Attachment #634362 -
Attachment is obsolete: true
Irakli, by the way, I have added one test with a hostile promise. You may wish to add this or a variant thereof to your testsuite.
Attachment #633498 -
Attachment is obsolete: true
(In reply to David Rajchenbach Teller [:Yoric] from comment #96) > Unfortunately, I still have one concern: I still really do not have have the > slightest clue of how to instrument Irakli's code to add the debugging > support I have needed so far when working with hostile-to-refactoring code. > > If Irakli assures me that this is feasible without major refactoring to his > code, I will happily let his patch land and move this point of the task to a > followup bug. Let's move this to a followup bug regardless. Designing the feature properly will take some time.
Assignee | ||
Comment 101•12 years ago
|
||
Thanks Dave! > I believe that the exercise was not a complete waste of time. I know that I have > learnt a few things along the way, and I believe that both implementations have > considerably improved during the course of this process. I agree, I have learned a lot from this process. > P.S.: I feel compelled to take some credit for Irakli's documentation, just as > Irakli deserves some credit for my code. As a matter of fact, I thin you should take full credit for that, as you've suggested and pushed into making all them. > If Irakli assures me that this is feasible without major refactoring to his code, I > will happily let his patch land and move this point of the task to a followup bug. I ensure you that all the debug logging you had in a fork can be done without major (if any) refactoring. > I have added one test with a hostile promise. You may wish to add this or a variant > thereof to your testsuite. Awesome I'll pull it in. In fact we could probably land rest of your test as well.
Assignee | ||
Comment 102•12 years ago
|
||
BTW I have not mentioned it here, but I did in pull request. I have addressed review comments, although I'm still waiting for feedback / suggestion on few disliked namings. Also we still have not reached agreement on observers notification order. Yoric, Mossop could you please provide your input so we can move this forward to completion. Thanks
Assignee: dteller → rFobic
Assignee | ||
Comment 103•12 years ago
|
||
Yoric: I don't know if that is still relevant or worth an effort, but I finally managed to complete an experiment based on our previous discussions & your suggestions: https://gist.github.com/2763402 I believe it provides more layers one can hook into. So if you and devtools code is not constrained with security concerns we have and prefer faster version you will be ale to use it. Again, I don't know if it's still worth it, but if you think it is you should probably speak up now :)
Reporter | ||
Updated•12 years ago
|
Attachment #633711 -
Flags: review?(jwalker)
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #103) > Yoric: I don't know if that is still relevant or worth an effort, but I > finally managed to complete an experiment based on our previous discussions > & your suggestions: > > https://gist.github.com/2763402 > > I believe it provides more layers one can hook into. So if you and devtools > code is not constrained with security concerns we have and prefer faster > version you will be ale to use it. > > Again, I don't know if it's still worth it, but if you think it is you > should probably speak up now :) We can probably live without it, thanks :)
Assignee | ||
Comment 105•12 years ago
|
||
I have addressed comments outlined in the pull request with except renames form `fulfill` to `resolve` for a reasons commented here: https://github.com/mozilla/mozilla-central/pull/4#discussion_r1071675 Also, there were multiple suggestions for name improvements like `rejected` and `rejectedFakePromise` I have chosen to go with a first one for reasons described here: https://github.com/mozilla/mozilla-central/pull/4#issuecomment-6641573 Although if you still think the later name is better let me know and I'll update to that.
Comment 106•12 years ago
|
||
Comment on attachment 633711 [details] Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/4 I think this is good to land now.
Attachment #633711 -
Flags: superreview?(dtownsend+bugmail) → superreview+
Comment 107•12 years ago
|
||
Landed this on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc419f436c8a We still need to figure out packaging it into the build and making it available to all consumers, it might be better to do that in other bugs now that this one is past 100 comments!
Comment 108•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc419f436c8a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in
before you can comment on or make changes to this bug.
Description
•