Closed
Bug 1093021
Opened 10 years ago
Closed 10 years ago
Implement PromiseUtils.defer
Categories
(Toolkit :: Async Tooling, defect)
Toolkit
Async Tooling
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: Yoric, Assigned: akshendra521994, Mentored)
References
Details
(Whiteboard: [lang=js][good next bug])
Attachments
(1 file, 2 obsolete files)
7.39 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
Much of our code is currently based on Promise.jsm, a soon-to-be-removed implementation of Promise. We are in the process of porting code that uses Promise.jsm to the DOM Promise, which is almost identical, but misses (by specification) function `Promise.defer`.
The objective of this bug is to add a function `defer` to PromiseUtils.jsm, with the same behavior as the `Promise.defer` defined in Promise.jsm, but using DOM Promise. This will help us progressively get rid of Promise.jsm.
Comment 1•10 years ago
|
||
I'd call this module "PromiseLegacy.jsm", but actually I'm not sure this is the right deprecation strategy for Promise.defer.
I found over 250 add-on files using Promise.defer from Promise.jsm and ancestors, generally trying to import each historical module in order:
Cu.import("resource://gre/modules/commonjs/promise/core.js");
Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
Cu.import("resource://gre/modules/Promise.jsm");
This deprecation strategy will likely just result in add-ons just adding PromiseLegacy.jsm to this list, and we would still need to maintain the legacy function in our codebase.
A different approach could be for the polyfill of "Promise.defer" to be declared privately in the modules of our codebase that use it. It is simple and can also be conveniently reduced to a one-liner:
Promise.defer=()=>(((o,p=new Promise((r,j)=>(o={resolve:r,reject:j})))=>(o.promise=p,o))());
Some modules might just switch to the "new Promise" syntax instead of adding the polyfill.
Add-ons would be encouraged to migrate to the "new Promise" syntax, or just include and maintain the polyfill in their code base.
Reporter | ||
Comment 2•10 years ago
|
||
Note that bug 1080466 will already introduce PromiseUtils.jsm.
Also, I have seen some code that is really nicer to write with `Promise.defer` (or `PromiseUtils.defer`) than `new Promise()`, so having `PromiseUtils.defer` makes sense just for the sake of readability in such cases. As far as I am concerned, the fact that we can use it to simplify transition to DOM Promise is a nice bonus.
Comment 3•10 years ago
|
||
I see, so you're actually proposing to introduce this as an API for writing "defer"-style code, that we support internally going forward, with no deprecation warnings on it. If this is correct, I don't have a strong opinion about it, can be a nice thing to have.
Assignee | ||
Comment 4•10 years ago
|
||
This is kind of a first sketch.
I have a query related to the rejects method here, what should happen when a Promise in pending or settled state is passed to the reject method of the Deferred object.
Attachment #8518440 -
Flags: feedback?(dteller)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8518440 [details] [diff] [review]
1093021_PromiseUtilsDefer.patch
Review of attachment 8518440 [details] [diff] [review]:
-----------------------------------------------------------------
A few nits, but generally, that looks good.
::: toolkit/modules/PromiseUtils.jsm
@@ +51,5 @@
> }, delay);
> });
> + },
> +
> + /*
Nit: whitespace.
@@ +52,5 @@
> });
> + },
> +
> + /*
> + * A implementation of Promise.defer using DOM Promise.
That first line is not useful.
@@ +55,5 @@
> + /*
> + * A implementation of Promise.defer using DOM Promise.
> + * Creates a new pending Promise and provide methods to resolve and reject this Promise.
> + *
> + * @return {Deferred} an object consiting of a pending Promise "promise"
A few typos in this sentence.
@@ +68,5 @@
> + * It contains a Promise and methods to change its state
> + */
> +
> +function Deferred() {
> + this.promise = new Promise((resolve, reject) => {
I would prefer if you documented the fields inside the constructor.
For clarity, you may wish to rewrite
function Deferred() {
/**
* Documentation of `resolve`, bla, bla, bla.
*/
this.resolve = null;
/**
* Documentation of `reject`, bla, bla, bla.
*/
this.reject = null;
/**
* Documentation of `promise`
*/
this.promise = new Promise(...)
}
/**
* Bla, bla, bla
*/
this.promise = new Promise(... => {
/**
* A method, bla, bla, bla.
*/
this.resolve = resolve;
});
@@ +74,5 @@
> + this.reject = reject;
> + });
> +}
> +
> +Deferred.prototype = {
I assume that you put the prototype here for documentation purposes. Since putting mutable values in a prototype is harmful, I prefer that you move the documentation inside the constructor and get rid of the prototype.
::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js
@@ +91,5 @@
> +add_task(function* test_resolve_string() {
> + let def = PromiseUtils.defer();
> + def.resolve("The promise is resolved");
> + let result = yield def.promise;
> + Assert.equal(result, "The promise is resolved", "def.resolve() resolves the promise");
Generally, you should define "The promise is resolved" as a constant, e.g.
let expected = "The promise is resolved " + Math.random();
...
Assert.equal(result, expected, ...);
@@ +104,5 @@
> + Assert.equal(result, undefined, "resolve works with undefined as well");
> +});
> +
> +/* Test when a pending promise is passed to the resolve method
> + * of the Deffered object */
Nit: "Deferred" (also in other places).
@@ +115,5 @@
> + let result = yield def.promise;
> + Assert.equal(result, 100, "def.promise assumed the state of the passed promise");
> +});
> +
> +/* Test when a resoled promise is passed
Nit: "resolved".
@@ +125,5 @@
> + let result = yield def.promise;
> + Assert.equal(result, "Yeah resolved", "Resolved promise is passed to the resolve method");
> +});
> +
> +/* Test for the case when a rejected promise is
Nit: whitespace.
Attachment #8518440 -
Flags: feedback?(dteller) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8518440 -
Attachment is obsolete: true
Attachment #8518850 -
Flags: review?(dteller)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8518850 [details] [diff] [review]
1093021_PromiseUtilsDefer.patch
Review of attachment 8518850 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, with a few trivial nits.
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b8f56fa4312
::: toolkit/modules/PromiseUtils.jsm
@@ +62,5 @@
> + return new Deferred();
> + },
> +}
> +
> +/* The definition of Deferred object which is returned by PromiseUtils.defer(),
/**
*
*/
@@ +63,5 @@
> + },
> +}
> +
> +/* The definition of Deferred object which is returned by PromiseUtils.defer(),
> + * It contains a Promise and methods to change its state
Nit: Not actually change the state, just resolve/reject it.
@@ +85,5 @@
> + */
> + this.reject = null;
> +
> + /* A newly created Pomise object
> + * Initially in pending state
Nit: dot at the end of sentences.
::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js
@@ +170,5 @@
> +add_task(function* test_reject_resolved_promise() {
> + let def = PromiseUtils.defer();
> + let p = new Promise((resolve, reject) => reject(new Error("This on rejects")));
> + def.reject(p);
> + yield Assert.rejects(def.promise, Promise, "Rejection with a rejected promise uses the passed promise itself as the reason of rejection");
Nit: whitespace.
Attachment #8518850 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8518850 -
Attachment is obsolete: true
Attachment #8519906 -
Flags: review?(dteller)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8519906 [details] [diff] [review]
1093021_PromiseUtilsDefer.patch
Review of attachment 8519906 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me.
Let's wait for the results above, then land the patch if they are positive.
Attachment #8519906 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Please assign to me.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → akshendra521994
Reporter | ||
Comment 11•10 years ago
|
||
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [lang=js][good next bug] → [lang=js][good next bug][fixed-in-fx-team]
Comment 13•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good next bug][fixed-in-fx-team] → [lang=js][good next bug]
Target Milestone: --- → mozilla36
Comment 14•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•