David Teller [:Yoric] - still alive but not very active Reporter
	Description • 10 years ago

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.

	:Paolo Amadini
	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.

	David Teller [:Yoric] - still alive but not very active 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.

	:Paolo Amadini
	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.

	Akshendra Pratap Singh 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.

	David Teller [:Yoric] - still alive but not very active 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.

	David Teller [:Yoric] - still alive but not very active 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.

	David Teller [:Yoric] - still alive but not very active 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.

	Akshendra Pratap Singh Assignee
	Comment 10 • 10 years ago

Please assign to me.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 11 • 10 years ago

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b8f56fa4312

	Carsten Book [:Tomcat]
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/8ab91f02f963

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8ab91f02f963

	Tooru Fujisawa [:arai]
	Comment 14 • 9 years ago

Updated following documentation pages:
  https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PromiseUtils.jsm
  https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PromiseUtils.jsm/Deferred
  https://developer.mozilla.org/en-US/Firefox/Releases/36