Closed Bug 1093021 Opened 5 years ago Closed 5 years ago

Implement PromiseUtils.defer

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: Yoric, Assigned: akshendra521994, Mentored)

References

Details

(Whiteboard: [lang=js][good next bug])

Attachments

(1 file, 2 obsolete files)

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.
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.
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.
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.
Attached patch 1093021_PromiseUtilsDefer.patch (obsolete) — Splinter Review
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)
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+
Attached patch 1093021_PromiseUtilsDefer.patch (obsolete) — Splinter Review
Attachment #8518440 - Attachment is obsolete: true
Attachment #8518850 - Flags: review?(dteller)
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+
Attachment #8518850 - Attachment is obsolete: true
Attachment #8519906 - Flags: review?(dteller)
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+
Please assign to me.
Assignee: nobody → akshendra521994
https://hg.mozilla.org/integration/fx-team/rev/8ab91f02f963
Keywords: checkin-needed
Whiteboard: [lang=js][good next bug] → [lang=js][good next bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8ab91f02f963
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good next bug][fixed-in-fx-team] → [lang=js][good next bug]
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.