Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Bug 723555 - Implementation of promise abstraction #350

Merged
merged 1 commit into from Apr 2, 2012

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Feb 23, 2012

This change change brings low level API for working with promises in the SDK.

https://bugzilla.mozilla.org/show_bug.cgi?id=723555

## Rationale

Most of the JS APIs are asynchronous complementing it's non-blocking nature.
While this is has a good reason and many advantages, it comes with a price.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will have to choose between is and has :p

@Gozala
Copy link
Contributor Author

Gozala commented Mar 2, 2012

Addressed all the review comments.


var bar = foo().then(function success(value) {
// ...
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two code examples aren't really helpfull. Both are identical whereas we try to explain different behaviors. I'm not sure examples are really important. These two sentences are quite clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First argument to then was supposed to be null.

reduce(promisedConcat, resolve([], prototype)).
// finally map that to promise of `f.apply(this, args...)`
then(execute)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using [f, this].concat(Array.slice(arguments)) instead?
or var args = Array.slice(arguments); [f, this].concat(args)
It highlights the issue of arguments, and we are used to Array.slice in our codebase.

Is it possible to process only arguments to reduce. It is really really hard to get why arguments and f/this are processed into the reduce function!? It would be way easier to follow with something like this:

  let self = this;
  return
    // create real array for `arguments`, with `reduce` method
    Array.slice(arguments).
    // reduce it via `promisedConcat` to get promised array of fulfillments
    reduce(promisedConcat, resolve([], prototype)).
    // finally map that to promise of array with `f`
    then(function(args) { return f.apply(self, args); });

Note that execute shortcut doesn't necessary improve comprehension of this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using [f, this].concat(Array.slice(arguments)) instead?
or var args = Array.slice(arguments); [f, this].concat(args)
It highlights the issue of arguments, and we are used to Array.slice in our codebase.

This pull request was importing library that is not written for moz-js in standard js you don't have Array.slice. Also slice is suboptimal for such hotspot codepaths. I have being doing diff profiling to identify fastest option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that execute shortcut doesn't necessary improve comprehension of this code.

No I agree it's harder to read this code, but it's way more optimal and this function something that is frequently used, so reducing closures introduced on each call is very important. Also see comments on line 162 that explains explain that.

@ochameau
Copy link
Contributor

ochameau commented Apr 2, 2012

r+, mainly nits. Feel free to request another round if you feel too, but I think it can land without a new one.
Please consider stashing the history before landing, as we do not really care about all back and forth revisions with various namings.

rejected for the same reason.

In the Add-on SDK we follow [CommonJS Promises/A]
(http://wiki.commonjs.org/wiki/Promises/A) specification and model a promise as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant this link. This link isn't recognized by the doc parser.

Gozala added a commit that referenced this pull request Apr 2, 2012
fix Bug 723555 - Implementation of promise abstraction r=@ochameau
@Gozala Gozala merged commit ee0cbde into mozilla:master Apr 2, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants