New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A+ compatible Promises #445

Merged
merged 18 commits into from Feb 15, 2013

Conversation

Projects
None yet
4 participants
@juandopazo
Member

juandopazo commented Feb 11, 2013

This is a refactor of @lsmith's code in #241. Previous discussions and the evolution of this code can be seen in juandopazo#4.

This pull request introduces a new module called promise instead of deferred. Promises are already a complex concept to understand and teach, and deferred objects make it harder. This refactor internally keeps a similar relationship between objects as in deferreds+promises, but it tries to hide it with a new API.

The new way to create promises is by using the Y.Promise constructor and passing it a function in which you initialize promise. For example:

/**
Waits for a number of milliseconds and returns a promise that gets
fulfilled when done waiting.

@param {Number} ms Number of milliseconds to wait
@return Promise
*/
Y.wait = function (ms) {
  return new Y.Promise(function (fulfill) {
    setTimeout(fulfill, ms);
  });
};

Underneath there's another object called Y.Promise.Resolver which does the same thing as the old Deferred.

A+ status

As far as interoperability goes the status of the A+ spec is very stable. There are other aspects that are still under discussion but are not essential to getting a compatible implementation. Mostly the A+ group is looking for the best API for creating promises and cancelling them. new Y.Promise(fn) was born in Microsoft's implementation for WinJS and mostly adopted by the A+ group, with some differences. This implementation follows the same pattern as the WinJS one.

Cancelling is a topic still under discussion and that it should at some point be added to the YUI implementation because some async operations are usually cancelled. For example: XHR and setTimeout.

Tests

I've added tests that cover the basic functionality of promises, when and batch. Tests for when and batch should be enough, but the ones in the A+ test suite are much more comprehensive. It'd be great to run them to the YUI test runs. They're written in Jasmine which doesn't help, but translating to YUI Test would double the effort or guarantee that they become outdated.

Running the A+ Test Suite

Steps for running the test suite:

  1. Create a folder somewhere
  2. Copy the package.json file that installs YUI and the tests
  3. Copy the test adapter file (see the same gist), build the Promise module and point YUI in the adapter to the correct location of that module
  4. Run npm install .
  5. Run npm test

Can we add the A+ tests as a CLI test? What would it take?

Docs

I'm starting to work on the docs. Nothing to see yet. I plan on:

  • Covering some background on promises as simple async operations chaining, value wrappers and async control flow managers
  • Showing how to create promises
  • Explaining how thenworks, how to chain promises and deal with failures
  • Explaining Y.when and Y.batch
  • Adding at least two examples. A simple one like Y.wait and a more complex one like writing your own Y.Node plugin that chains transitions.

juandopazo added some commits Feb 11, 2013

Initial commit
For development history see juandopazo#4
Functions are no longer special params in batch
Y.batch parameters are just run through Y.when to simplify how it works
Show outdated Hide outdated src/promise/js/extras.js Outdated
Show outdated Hide outdated src/promise/js/extras.js Outdated
Show outdated Hide outdated src/promise/js/extras.js Outdated
Show outdated Hide outdated src/promise/js/extras.js Outdated
@juandopazo

This comment has been minimized.

Show comment
Hide comment
@juandopazo

juandopazo Feb 11, 2013

Member

Let's remove extras from this release. That way we can:

  • Learn from the cancellation discussion at A+
  • Take time to discuss if this should be in an "extras" module or be included by default
Member

juandopazo commented Feb 11, 2013

Let's remove extras from this release. That way we can:

  • Learn from the cancellation discussion at A+
  • Take time to discuss if this should be in an "extras" module or be included by default
@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Feb 11, 2013

Member

I've added tests that cover the basic functionality of promises, when and batch. Tests for when and batch should be enough, but the ones in the A+ test suite are much more comprehensive. It'd be great to run them to the YUI test runs. They're written in Jasmine which doesn't help, but translating to YUI Test would double the effort or guarantee that they become outdated.

[...]

Can we add the A+ tests as a CLI test? What would it take?

@reid or @davglass Any insight on the above ^ ? We'd like to be able to "import" the standardized test suite and run it against our implementation, but we don't want to re-write the tests as YUI Test tests. Do you guys know of a way we can easily import, or run these tests but have it output to our standard reporting? I know there's been work on Yeti to do this sort of thing.

Member

ericf commented Feb 11, 2013

I've added tests that cover the basic functionality of promises, when and batch. Tests for when and batch should be enough, but the ones in the A+ test suite are much more comprehensive. It'd be great to run them to the YUI test runs. They're written in Jasmine which doesn't help, but translating to YUI Test would double the effort or guarantee that they become outdated.

[...]

Can we add the A+ tests as a CLI test? What would it take?

@reid or @davglass Any insight on the above ^ ? We'd like to be able to "import" the standardized test suite and run it against our implementation, but we don't want to re-write the tests as YUI Test tests. Do you guys know of a way we can easily import, or run these tests but have it output to our standard reporting? I know there's been work on Yeti to do this sort of thing.

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 11, 2013

Member

I just glanced at this, but it looks like we should be able to write a YUI adapter to this. I can take a look later when I have some free time.

Member

davglass commented Feb 11, 2013

I just glanced at this, but it looks like we should be able to write a YUI adapter to this. I can take a look later when I have some free time.

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Feb 11, 2013

Member

Thanks @davglass. That will be ideal so we can make sure we continue to adhere to the A+ spec's tests.

Member

ericf commented Feb 11, 2013

Thanks @davglass. That will be ideal so we can make sure we continue to adhere to the A+ spec's tests.

@lsmith

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Feb 11, 2013

Contributor

+1 for leaving out extras for this release

Contributor

lsmith commented Feb 11, 2013

+1 for leaving out extras for this release

Remove extras module from this release
This way we can enhance cancel() in the future and possibly provide
timeout() by default
Show outdated Hide outdated src/promise/js/promise.js Outdated
Show outdated Hide outdated src/promise/js/resolver.js Outdated
Show outdated Hide outdated src/promise/js/promise.js Outdated
Show outdated Hide outdated src/promise/js/resolver.js Outdated
Show outdated Hide outdated src/promise/meta/promise.json Outdated
Remove the dependency on "oop"
Replaced the use of bind() with a closure to avoid a utility module and
for extra performance.
Show outdated Hide outdated src/promise/js/promise.js Outdated
@lsmith

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Feb 12, 2013

Contributor

@davglass will you be merging this, or do you want me or @ericf to?

Contributor

lsmith commented Feb 12, 2013

@davglass will you be merging this, or do you want me or @ericf to?

@juandopazo

This comment has been minimized.

Show comment
Hide comment
@juandopazo

juandopazo Feb 12, 2013

Member

One last point for discussion: since this is not going to be included in YUI by default but rather be an extra utility, shouldn't we include batch in the promise module? It's one of the key utilities for dealing with promises.

Member

juandopazo commented Feb 12, 2013

One last point for discussion: since this is not going to be included in YUI by default but rather be an extra utility, shouldn't we include batch in the promise module? It's one of the key utilities for dealing with promises.

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 12, 2013

This might read better as a switch statement since you are only checking for 2 values:

        switch (this._status) {
            case 'fulfilled':
                this.fulfill(this._result);
                break;
            case 'rejected':
                this.reject(this._result);
                break;
        }

davglass commented on src/promise/js/resolver.js in 6d92e15 Feb 12, 2013

This might read better as a switch statement since you are only checking for 2 values:

        switch (this._status) {
            case 'fulfilled':
                this.fulfill(this._result);
                break;
            case 'rejected':
                this.reject(this._result);
                break;
        }

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Feb 12, 2013

I don't see a benefit here, but then again, I hate switch statements.

lsmith replied Feb 12, 2013

I don't see a benefit here, but then again, I hate switch statements.

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 12, 2013

For me it's easier to read :)

And it's way easier to get branch coverage on this too. With the if/else if approach you have to do much more work to test both the if and the else case. And I want this feature to be 100% across the board if it's to be a core feature that others rely on.

davglass replied Feb 12, 2013

For me it's easier to read :)

And it's way easier to get branch coverage on this too. With the if/else if approach you have to do much more work to test both the if and the else case. And I want this feature to be 100% across the board if it's to be a core feature that others rely on.

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 12, 2013

Member

@juandopazo I would feel better if you could spend a little more time getting this to a solid 100% across the board (line, branch, function, statement with yogi serve --istanbul or yogi test --coverage --istanbul) with the tests. If this is going to be relied upon by other parts of the system I would like to make sure it's very well tested and totally covered.

I spent about 10 minutes on it and got it up to: 97.18%, 93.75% (30 / 32), 100% (20 / 20), 97.18% so it shouldn't be too hard or time consuming :)

Member

davglass commented Feb 12, 2013

@juandopazo I would feel better if you could spend a little more time getting this to a solid 100% across the board (line, branch, function, statement with yogi serve --istanbul or yogi test --coverage --istanbul) with the tests. If this is going to be relied upon by other parts of the system I would like to make sure it's very well tested and totally covered.

I spent about 10 minutes on it and got it up to: 97.18%, 93.75% (30 / 32), 100% (20 / 20), 97.18% so it shouldn't be too hard or time consuming :)

juandopazo added some commits Feb 13, 2013

Test for the behavior of then()
There needed to be test for the then() callback returning a promise or
throwing an error.
Also added test for the optional callbacks in Y.when()
Remove getResult(), unnecessary test in _notify
Resolver#getResult was unnecesesary since we're not exposing it to the
promise and is not necessary for internal use.
A check for the existance of "subs" in _notify was a left over from the
previous version in which some callback arrays could be gone. Now that
array always exists.
@juandopazo

This comment has been minimized.

Show comment
Hide comment
@juandopazo

juandopazo Feb 13, 2013

Member

I learned the power of yogi serve --istanbul and got it to 100%.

Member

juandopazo commented Feb 13, 2013

I learned the power of yogi serve --istanbul and got it to 100%.

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 13, 2013

Member

Great! Thanks..

Member

davglass commented Feb 13, 2013

Great! Thanks..

Show outdated Hide outdated src/promise/HISTORY.md Outdated
Show outdated Hide outdated src/promise/README.md Outdated
Show outdated Hide outdated src/promise/js/batch.js Outdated
Show outdated Hide outdated src/promise/js/batch.js Outdated
Show outdated Hide outdated src/promise/js/promise.js Outdated
Show outdated Hide outdated src/promise/js/promise.js Outdated
Show outdated Hide outdated src/promise/js/resolver.js Outdated
Show outdated Hide outdated src/promise/js/resolver.js Outdated
Show outdated Hide outdated src/promise/js/resolver.js Outdated
Show outdated Hide outdated src/promise/js/resolver.js Outdated
Show outdated Hide outdated src/promise/meta/promise.json Outdated
@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Feb 14, 2013

Member

@juandopazo Looks awesome!

I added a few nitpick comments, mainly around docs stuff. Also don't be afraid to add code comments which explain why things are a certain way in the non-obvious places. Certain methods look like they have code comments, other ones which are have some not completely obvious procedures do not. This said, you don't have to go crazy, just in the places where another developer might get tripped up.

Member

ericf commented Feb 14, 2013

@juandopazo Looks awesome!

I added a few nitpick comments, mainly around docs stuff. Also don't be afraid to add code comments which explain why things are a certain way in the non-obvious places. Certain methods look like they have code comments, other ones which are have some not completely obvious procedures do not. This said, you don't have to go crazy, just in the places where another developer might get tripped up.

juandopazo added some commits Feb 14, 2013

Add example to Promise constructor docs
Also comment on why duck typing is used to determine if an object is a
promise
Show outdated Hide outdated src/promise/js/resolver.js Outdated
Rename callback lists to _callbacks and _errbacks
Added extra API docs for callback lists and inline comments to explain
why they're reset each time fulfill/reject are called
@juandopazo

This comment has been minimized.

Show comment
Hide comment
@juandopazo

juandopazo Feb 15, 2013

Member

Latest changes:

  • Renamed iterator counter from j to i in _notify
  • Added an example of how to use the Promise constructor in the API Docs
  • Added inline comments about the reason for duck typing when detecting promises in isPromise
  • Renamed _fulfillSubs and _rejectSubs to _callbacks and _errbacks
  • Added comments explaining why callback lists are reset with each use

I'll try to write as much as I can of the User Guide during the weekend. I have @lsmith's graphics to illustrate it which is awesome. I'll probably ask you guys for help next week. It'll be my first User Guide and touching a fairly difficult topic to explain.

Member

juandopazo commented Feb 15, 2013

Latest changes:

  • Renamed iterator counter from j to i in _notify
  • Added an example of how to use the Promise constructor in the API Docs
  • Added inline comments about the reason for duck typing when detecting promises in isPromise
  • Renamed _fulfillSubs and _rejectSubs to _callbacks and _errbacks
  • Added comments explaining why callback lists are reset with each use

I'll try to write as much as I can of the User Guide during the weekend. I have @lsmith's graphics to illustrate it which is awesome. I'll probably ask you guys for help next week. It'll be my first User Guide and touching a fairly difficult topic to explain.

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 15, 2013

Member

Thanks for all the hard work. I'll pull this down & merge it with my
working branch to get this in the tree.

Member

davglass commented Feb 15, 2013

Thanks for all the hard work. I'll pull this down & merge it with my
working branch to get this in the tree.

@lsmith

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Feb 15, 2013

Contributor

\o/ promise all the things!

Contributor

lsmith commented Feb 15, 2013

\o/ promise all the things!

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 15, 2013

Member

Merging this in now, @juandopazo you can work on the docs on a different Pull Request dedicated to just docs.

For posterity:

243 tests complete (6 seconds) //The promises-aplus-tests suite
.
Total tests: 23, Failures: 0, Skipped: 0, Time: 5.856 seconds

yogi [info]  yuitest tests complete
yogi [info]  turning on coverage support in grover
Starting Grover on 1 files with PhantomJS@1.8.1
  Running 15 concurrent tests at a time.
✔ [Promise]: Passed: 22 Failed: 0 Total: 22 (ignored 0) (0.545 seconds) 100%

-----------------+-----------+-----------+-----------+-----------+
File             |   % Stmts |% Branches |   % Funcs |   % Lines |
-----------------+-----------+-----------+-----------+-----------+
   promise/      |       100 |       100 |       100 |       100 |
      promise.js |       100 |       100 |       100 |       100 |
-----------------+-----------+-----------+-----------+-----------+
All files        |       100 |       100 |       100 |       100 |
-----------------+-----------+-----------+-----------+-----------+


=============================== Coverage summary ===============================
Statements   : 100% ( 83/83 )
Branches     : 100% ( 38/38 )
Functions    : 100% ( 23/23 )
Lines        : 100% ( 83/83 )
================================================================================
----------------------------------------------------------------
✔ [Total]: Passed: 22 Failed: 0 Total: 22 (ignored 0) (0.545 seconds)
  [Grover Execution Timer] 1.931 seconds

Member

davglass commented Feb 15, 2013

Merging this in now, @juandopazo you can work on the docs on a different Pull Request dedicated to just docs.

For posterity:

243 tests complete (6 seconds) //The promises-aplus-tests suite
.
Total tests: 23, Failures: 0, Skipped: 0, Time: 5.856 seconds

yogi [info]  yuitest tests complete
yogi [info]  turning on coverage support in grover
Starting Grover on 1 files with PhantomJS@1.8.1
  Running 15 concurrent tests at a time.
✔ [Promise]: Passed: 22 Failed: 0 Total: 22 (ignored 0) (0.545 seconds) 100%

-----------------+-----------+-----------+-----------+-----------+
File             |   % Stmts |% Branches |   % Funcs |   % Lines |
-----------------+-----------+-----------+-----------+-----------+
   promise/      |       100 |       100 |       100 |       100 |
      promise.js |       100 |       100 |       100 |       100 |
-----------------+-----------+-----------+-----------+-----------+
All files        |       100 |       100 |       100 |       100 |
-----------------+-----------+-----------+-----------+-----------+


=============================== Coverage summary ===============================
Statements   : 100% ( 83/83 )
Branches     : 100% ( 38/38 )
Functions    : 100% ( 23/23 )
Lines        : 100% ( 83/83 )
================================================================================
----------------------------------------------------------------
✔ [Total]: Passed: 22 Failed: 0 Total: 22 (ignored 0) (0.545 seconds)
  [Grover Execution Timer] 1.931 seconds

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass
Member

davglass commented Feb 15, 2013

@davglass davglass merged commit a51978d into yui:dev-3.x Feb 15, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment