Promises extras update

[juandopazo]

This pull request splits the promise module into two modules: promise-core and promise-extras. The promise name is still an alias that loads these two new modules. The core module has little changes and the extras module introduces new features.

Changes from previous version

• Promises now flatten other promises in a resolution procedure located in resolver.resolve
• The first argument in the promise initialization is now a reference to resolver.resolve
• promise.getStatus is now only available when including the promise-extras module
• promise.then no longer returns an instance of promise.constructor. Instead it always return a bare Promise with no subclassing
• Y.batch is deprecated in favor of Y.Promise.every. Resolves [Promise] Y.batch should accept and flatten nested arrays #740
• Resolves Add conditional to Promise.Resolver.then method for efficiency when wrapping callbacks and errbacks #571

New features

• New method: promise.done(). done behaves in a simlar way to then, but it doesn't return a new promise and it doesn't catch errors to turn them into rejections. `fail is a
• New factory methods Promise.resolve and Promise.reject for easier creation of already resolved promises
• New combinator methods Promise.every, Promise.any and Promise.some. Resolves Y.batch() and other Promise related methods should be static on Y.Promise #670
• I did not include Promise.accept from the DOM standard because it opens the door for promises for promises which I don't think we want unless proven useful. It can be added later.

Changes in the docs

• Removed all references to batch from the docs.
• Removed the "Subclassing promises" example. Subclassing is already covered by the Plugin example and I don't see a simple-to-explain non-hacky way of showing how to write a LazyPromise
• Changed all uses of Promise(function (fulfill) ... to using "resolve"

Promises/A+

• These changes should make our implementation mostly compatible with A+ 1.1. The new A+ version hasn't been ratified yet. There is still the open issue of resolving promises/thenables that are fulfilled with themselves.

Test results

• Fixes Promise flaky test #637 by making the test synchronous and so avoiding race conditions.

Starting Grover on 2 files with [email protected]
  Running 15 concurrent tests at a time.
OK [Promise core tests]: Passed: 14 Failed: 0 Total: 15 (ignored 1) (0.334 secon
ds) 100%
OK [Promise extras]: Passed: 37 Failed: 0 Total: 37 (ignored 0) (1.108 seconds)
100%

------------------------+-----------+-----------+-----------+-----------+
File                    |   % Stmts |% Branches |   % Funcs |   % Lines |
------------------------+-----------+-----------+-----------+-----------+
   promise-core\        |       100 |     92.86 |       100 |       100 |
      promise-core.js   |       100 |     92.86 |       100 |       100 |
   promise-extras\      |       100 |       100 |       100 |       100 |
      promise-extras.js |       100 |       100 |       100 |       100 |
------------------------+-----------+-----------+-----------+-----------+
All files               |       100 |     96.77 |       100 |       100 |
------------------------+-----------+-----------+-----------+-----------+

I'll work on getting that 100% on branches back.

I created a new branch with no commit history because I went back and forth a bit with some changes and I reached nearly 30 commits.

[lsmith]

I'll look this over today or tomorrow.

[lsmith]

It's worth including a section in the docs or a blog post explaining the changes. In particular, the removal of subclassing support and the deprecation of Y.batch. The Y.batch change should be noted in the user guide for at least as long as the deprecation is in effect prior to removal. It doesn't have to be used in example code, but a deprecation notice is worthwhile, especially because a signature change is required.

[lsmith]

• The other combinators should be documented in the user guide.

[lsmith]

Can this fork be moved out of the Promise constructor and return a static pre-resolved promise?

[lsmith]

• verify values was passed to avoid the TypeError.

[juandopazo]

I intentionally avoided a check there because I want it to throw an early error, but I didn't want to add extra code like !isArray(values) && throw new TypeError(...).

[lsmith]

A truthy check is all I had in mind. I wouldn't want to bog down the methods with isArray, either.

[juandopazo]

Getting back to this, I prefer it to throw. A wrong parameter here is a programmer error and as such it should fail fast. In fact, I think I should add

if (!Y.Lang.isArray(values)) { Y.log('TypeError: wrong parameter type', 'error'); }

...because if you pass a non null|undefined|arraylike value the result will be a forever pending promise, a hard thing to debug.

[juandopazo]

I'm starting to think I should split this kind of work into more specific pull requests rather than one big PR.

[lsmith]

Smaller PRs will get reviewed more thoroughly and promptly, for sure. But this one is ok as is.

[juandopazo]

I made the following changes that you should review:

• Simplified Y.when() by using Promise.resolve(). It maintains the same semantics, but the byte count should be a bit down
• Added a try...catch block in Promise.isPromise() for future compatibility with Promises/A+ 1.1
• Added an early return to resolver.resolve() to avoid calling then() unnecessarily in some cases
• Used an already resolved promise when the combinator functions receive an empty list

I also added most of the tests we discussed. I worked around the race condition in Promise.every() by resolving one of the promise synchronously and the rest asynchronously.

I still need to add the tests for the combinator functions without params. I'm hoping we can reach an agreement on that.

Coverage is back to 100%.

Starting Grover on 2 files with [email protected]
  Running 15 concurrent tests at a time.
OK [Promise core tests]: Passed: 16 Failed: 0 Total: 17 (ignored 1) (0.389 secon
ds) 100%
OK [Promise extras]: Passed: 38 Failed: 0 Total: 38 (ignored 0) (0.938 seconds)
100%

------------------------+-----------+-----------+-----------+-----------+
File                    |   % Stmts |% Branches |   % Funcs |   % Lines |
------------------------+-----------+-----------+-----------+-----------+
   promise-core\        |       100 |       100 |       100 |       100 |
      promise-core.js   |       100 |       100 |       100 |       100 |
   promise-extras\      |       100 |       100 |       100 |       100 |
      promise-extras.js |       100 |       100 |       100 |       100 |
------------------------+-----------+-----------+-----------+-----------+
All files               |       100 |       100 |       100 |       100 |
------------------------+-----------+-----------+-----------+-----------+


=============================== Coverage summary ===============================

Statements   : 100% ( 132/132 )
Branches     : 100% ( 64/64 )
Functions    : 100% ( 44/44 )
Lines        : 100% ( 132/132 )
================================================================================

----------------------------------------------------------------
OK [Total]: Passed: 54 Failed: 0 Total: 55 (ignored 1) (1.327 seconds)
  [Grover Execution Timer] 3.642 seconds

[lsmith]

Oh! I just found this comment. I'll take a look at this on Monday.

[juandopazo]

No worries. We were under code freeze, so I also put it on hold. I still need to work on the docs changes. I also want to add to the docs a short section about the reasoning behind having separate promise and resolver objects. I've been asked a lot of questions about that.

[lsmith]

As of the date of this comment, promises aren't flattened, only thenables. Promise.isPromise(x) returns false positives for unknown thenables, so this treats promises and thenables identically, which doesn't satisfy 1.1.

I'm open to suggestions on how to reconcile the issue. IMO, Promise.isPromise(x) should return true for instances of Y.Promise from another YUI instance or another window.

[juandopazo]

Yeah I was following DOM Promise here. I think we need to discuss branding again. If the whole point of A+ is to provide interoperability it'd a shame not to be able to identify other valid promises implementations.

BTW I've been meaning to review this section again. I think I'm creating too many anonymous functions so it may be a good idea to define resolve and reject in the instance.

[juandopazo]

No, that's not a good idea: http://jsperf.com/fn-wrap-vs-nowrap

[juandopazo]

Adding isThenable and having resolver.resolve behave differently for YUI promises. Tests should fail. Adding new tests afterwards.

[juandopazo]

@lsmith I misread the spec and it is using multiple arguments instead of an array, so that nulls are argument about empty lists for combinator methods. I mentioned it to Domenic and Alex Russell but it seems that it'll stay that way. See slightlyoff/Promises#70

I'll document the other combinators and send this off so I can get to work on synchronous resolution and other performance improvements.

[lsmith]

Oh boy, that sucks. varargs is a terrible idea. I saw the same thing right away when using Y.batch().

[juandopazo]

I know. I want to leave the door open for conditional loading in the
future, so I really want it to follow the spec. I've been considering
adding Y.Promise.all() which takes an array and calls every() as a YUI
addition.

[lsmith]

That follows Q.all(), but there's a chance others on the YUI team might be hesitant about using all() because of Y.all() and node.all(). I don't think it's a problem.

Can you bring that up in yui-contrib or the weekly round table?

[rgrove]

If the intent is to mirror Y.Array.every(), why not name it Y.Promise.every()? Or is there a conflict with the spec?

[juandopazo]

@rgrove the issue we're discussing is that Promise.every() is spec'd with multiple arguments and many have asked me to make Y.batch() flatten arrays or take an array so that they don't have to use apply().

[juandopazo]

I really want to make this a polyfill for ES/DOM promises. We'll probably see them in Firefox in the next months. That's why I don't want to introduce new methods that won't be in the ES/DOM spec. And while most of the features this PR introduces will be in the spec, the names of all methods and details of their implementation are still shifting.

So I'll close this PR and send more atomic PRs. The first one will probably just be a cleanup to reduce the number of objects created in each iteration and to add a first implementation of resolve().

If anyone wants more details just let me know and I'll post to yui-contrib.