Skip to content
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

Y.Deferred, yo. Let's make this happen! #241

Closed
wants to merge 10 commits into from
Closed

Conversation

lsmith
Copy link
Contributor

@lsmith lsmith commented Sep 6, 2012

Adds Y.Deferred and Y.Promise in 'deferred' module and features and Y.when (like Y.Parallel) in 'deferred-extras'.

Aimed to serve as the standard API for a transaction object/layer for big IO refactor. API feedback still welcome, though there was much discussion lsmith#36.

With some real tests, some stubbed tests. Basic functionality
seems to be working, but I haven't been very rigorous about
it yet.
New version uses prototypes and gentleman's privates, and
is broken into two pieces:
1. `deferred` which offers Y.Deferred and Y.Promise with only
   deferred.then(), resolve(), reject(), promise(), and getStatus(),
   and promise.then() and promise().
2. `deferred-extras` which adds deferred/promise.onProgress() and
   deferred.notify() for progress updates, an promise.wait() to
   insert a delay in the chained sequence.

Also added `deferred-when` which creates a `Y.when()` method to wrap
multiple operations and returns a promise that will be resolved when
all individual operations complete.
Split up the code into two modules:
deferred - core implementation
deferred-extras - +onProgress/notify, wait, and Y.when
Plus when() now supports passing simple values as well as
functions and promises.  Previously, it would never resolve
the allDone deferred if a simple value was passed.
@juandopazo
Copy link
Member

Fantastic! The only thing missing is what I mention in that comment I left: a mechanism to return objects extending Y.Promise so that we can have a Y.Transaction class for IO that extends Y.Promise, a NodeDeferred plugin or any other creative use we come up with.


this._promise = new Y.Promise(this);

this._status = 'in progress';
Copy link
Member

Choose a reason for hiding this comment

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

Would a single word status be better? "progress".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. I'm open to it, if you think it's important. To me, "progress" implies something has happened, which isn't necessarily the case. "unfulfilled" would be more appropriate, but is an odd word and would be typoed.

Copy link
Member

Choose a reason for hiding this comment

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

I think a single word is important, and I agree that English sucks.

Copy link

Choose a reason for hiding this comment

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

I agree that a single word would be best. pending?

Copy link
Member

Choose a reason for hiding this comment

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

I can't resist a good naming discussion. "inprogress"?

Copy link
Contributor

Choose a reason for hiding this comment

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

¿progresando?

Copy link
Member

Choose a reason for hiding this comment

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

"bobbingforapples"

Copy link
Contributor

Choose a reason for hiding this comment

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

Sold! You had me at "¿".

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to like pending or unfulfilled.

@ericf
Copy link
Member

ericf commented Sep 10, 2012

@lsmith This stuff looks like it will be a great API to build data-access stuff on!

I'm wondering about a couple of things…

  1. Do you look at Y.Deferred as a lower-level tool that wouldn't end up being exposed to the main end uses of YUI? I worry that it detracts from the YUI-way of doing things by not using events and attributes. That said I think it's important for a low-level API to remain lightweight.
  2. Double vs. single callback API. I'd like the discuss further the differences between using a Node.js style callback(err, data) API vs. the callback and errorback functions.

Also, I should probably do some deeper reading on the deferred/promise stuff, do you have any good links to share?

@lsmith
Copy link
Contributor Author

lsmith commented Sep 11, 2012

  1. The primary interface is the then(callback, errback) function of the promise and the resolve() and reject() methods on the deferred. Anything else is sugar. That said, I am still holding on to my initial implementation that used on('progress', callback) instead or onProgress(callback), which then bled over to promise.on(<"resolve" or "success" or "done">, callback) (and so for "reject", "failure", etc), which could be expanded per class specialization. But as an abstract API, then(), resolve(), and reject() would be the baseline, and be public. But yes, I really didn't want to require custom events for this layer because of the module dep weight and code complexity. Deferreds really should be a simple, (fairly?) inflexible API.
  2. I thought we'd talked about using Node.js style callback(err, data) for the transport layers, promises for transactions, and classes for transaction factories (aka DataSource or Resource). That's still my thought. Transport callbacks would dovetail into transactions by forking err => reject().

* More docs in then()
* errback that doesn't return a failed promise will continue to next callback,
  not next errback.  Signals recovery from the failure.
* Y.when renamed Y.batch
* New Y.when(promiseOrValue, callback, errback) added
* New Y.defer(callback(deferred)) added
* Tests added for new APIs and changed APIs
@ericf ericf mentioned this pull request Oct 11, 2012
@ericf
Copy link
Member

ericf commented Oct 15, 2012

Here's a good post about the true point of Promises:
https://gist.github.com/3889970

@lsmith
Copy link
Contributor Author

lsmith commented Oct 15, 2012

Yep, and it supports my want for keeping the Deferred and Promise separate. I am technically exposing the promise's underlying deferred via promise._deferred, which I had issue with, but seemed pragmatic instead of building a new object with unique methods closing over the deferred each time. The success and exception flow is supported in my implementation, though I don't think I'm handling literal exceptions via try/catch. He made no comment on the relevance or importance of progress handlers, as seems to be the usual case.

@juandopazo
Copy link
Member

Nice article! I found a possible bug based on that article:

// foo.json contains {items:[1,2,3]}
Y.io.get('foo.json')
    .then(function (id, xhr) {
        return JSON.parse(xhr.responseText);
    })
    .then(function (data) {
        return data.items.map(Math.sqrt);
    })
    .then(function (roots) {
        console.log(roots);
    }, function (id, xhr) {
        console.error(xhr);
    });

This works ok until returning an array from the second then(). Then it turns the array into parameters, so roots ends up being 1 instead of [1, 1.4142135623730951, 1.7320508075688772].

Maybe assuming an array means an argument list is not a good idea.

ms = Math.max(0, +ms || 0);

this.then(function () {
var args = slice.call(arguments);
Copy link
Member

Choose a reason for hiding this comment

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

No need to call slice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary to pass the then() args into the setTimeout function.

Copy link
Member

Choose a reason for hiding this comment

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

I mean you can just do var args = arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh duh! Right you are :)

Copy link
Member

Choose a reason for hiding this comment

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

There are a couple of those around. Must.save.CPU.cycles.

@ericf
Copy link
Member

ericf commented Oct 16, 2012

Another data point, RSVP lib by Yehuda Katz and Tom Dale:
https://github.com/tildeio/rsvp.js

@lsmith
Copy link
Contributor Author

lsmith commented Oct 16, 2012

Another implementation that ignores progress events. There's a discussion going on in the cujojs google group about propagating progress events down the promise chain. I seem to be on the "grumpy old man" side. shrug
https://groups.google.com/forum/?fromgroups=#!topic/cujojs/QurUiAeTa5E

@lsmith
Copy link
Contributor Author

lsmith commented Oct 24, 2012

Update: I've been chatting with @briancavalier, who is working on better defining the Promises/A spec (yay!), and doing more research. This implementation will change, but I don't anticipate much change in the API.

Look forward to more commits in the PR.

@ghost ghost assigned ericf Oct 25, 2012
@juandopazo
Copy link
Member

Hey @lsmith do you think we should rewrite Domenic's test suite for YUI Test or can we run it as a CLI test?

@juandopazo
Copy link
Member

Now that the promises A+ spec is stable.I drafted an implementation that implements that spec following the YUI style. I'm currently writing unit tests.

You can find it in a pull request in my own fork of YUI: juandopazo#4

There are some open issues:

  • Promises require Y.soon which means requiring “timers” every time we want promises
  • For tests, then() has a problem because it catches errors and so Y.Assert functions don’t work. @ericf mentioned the possibility of a workaround with YUI Test. Otherwise we may need end() a method that causes promises to throw errors
  • I still need to figure out how to return different types of promises without creating too many of them. See this gist to get an idea why this is an issue: https://gist.github.com/4506753
  • I need to translate the spec tests to YUI tests. This duplicates efforts but provides coverage. Should we run both versions of the spec tests?
  • Having compatibility with WinJS promises with a syntax like new Promise(function (fulfill, reject) {}) means creating two more functions per promise by using bind, and now that we're completely hiding the resolver from the promise by closing over it with instance properties we may want to avoid creating more functions and just pass the resolver to that function. Basically:
var promise = new Y.Promise(function (fulfill, reject) {
  fulfill('yay!');
});

// vs

var promise = new Y.Promise(function (resolver) {
  resolver.fulfill('yay!');
});

@lsmith
Copy link
Contributor Author

lsmith commented Jan 12, 2013

Issue responses:

  • What specifically are you concerned about? Timers is small, so not size. It's another function hop between promises and setImmediate, but that cost is unavoidable unless we want to standardize on setTimeout, which we don't.
  • I'm curious about the specific cases where this causes trouble. I don't think we need end(), though it would make a reasonable extras module/feature.
  • commented on the gist. Not sure this is a problem.
  • I'd say keep the non-YUI Test version of the tests in tests/manual
  • I'm partial to the function (fulfill, reject) syntax, and not concerned about the private functions. I don't think you need to use bind. You could do away with the Resolver class and set this._resolver = { fulfill: function ()..., reject: function ()... }; if necessary. This point warrants more discussion, though, for sure.

@juandopazo
Copy link
Member

Timers: you brought it up last time we chatted about this. I'm not that concerned. I'd like to have Y.Get return promises and that would mean bundling timers and promises in yui.js, but I can live with it being optional.

end: I only ran into this when writing tests. For instance:

'some asynchronous test for promises': function () {
  var test = this;

  Y.when(5).then(function (value) {
    // pausing and resuming work ok
    test.resume();

    // Assert.areEqual will throw an error which will be caught
    // by then() and reject this promise instead, so Y.Test
    // doesn't receive the assertion
    Y.Test.Assert.areEqual(4, value, 'value should be 4');
  });
  test.wait();
};

I absolutely prefer not to add end. I'll look at the internals of Y.Test to see if there's a workaround. Eric said something about there being an option that could help.

Syntax: I also like function (fulfill, reject). I don't usually worry about memory performance but since it's really easy to create a lot of Promise objects it's something to consider. Plus, there are cases in which you may want to use other methods from the resolver like getStatus. How would you expose them? At the A+ group, there's the proposal of having fulfill contain extra methods. But that means even more closures instead of prototype functions.

@juandopazo
Copy link
Member

Another issue: I'm not sure Y.batch should treat functions differently. There isn't much difference between Y.batch(fn) and Y.batch(Y.Promise(fn)). Maybe it made sense when we had to create a Deferred object first. And some people might want to return functions for some reason. It's JavaScript and it has functions as values after all. I think we should minimize magic here.

@briancavalier
Copy link

Hey guys, you've probably been following the resolver API convo over at Promises/A+, but just wanted to give a heads up that "fulfill" as a verb is being called into question. I'm currently against it, as are others, at least as it's been described to this point. I am strongly in favor the behavior of when.js and Q's resolve() (even if we change the name).

There's a new thread on the name here

@juandopazo
Copy link
Member

Nevermind about tests and end. I totally forgot how to use pause and resume in YUI Test. I was doing test.resume(); Assert.foo() instead of test.resume(function () { Assert.foo() });. * facepalm *

@briancavalier thanks for the heads-up! I agree that not being upfront with what fulfill does could lead to unexpected errors. For the others to follow what Brian says, here's an example:

// the identity function should work in any `then` callback
function identity(x) {
  return x;
}
// for example:
Y.when(5).then(identity).then(function (value) {
  console.log(value); // 5!
});

// however, if the value is a promise, it can give you an unexpected result
var promise = Y.Promise(function (fulfill) {
  // promise holds a promise for 5 as a value (a promise inside a promise)
  fulfill(Y.when(5));
});
// now we create a new promise going through identity
promise.then(identity).then(function (value) {
  // but we get 5, not a promise, because identity returned a promise to 
  // then() and then() does special stuff to promises
  console.log(value); // 5!
});

@lsmith
Copy link
Contributor Author

lsmith commented Jan 14, 2013

@juandopazo less magic for functions passed to Y.batch is fine. I honestly don't follow what the confusion is that you're trying to illustrate in your last code snippet. It all looks like expected behavior to me. Maybe I'm too close to it.

@briancavalier Thanks for the heads up. I've been remiss in keeping up with the org discussions, but will catch up tomorrow (Monday).

@solmsted
Copy link
Contributor

@juandopazo I don't quite understand what you are saying about Y.batch. Currently it accepts functions or promises as arguments, are you saying that you want it to only accept promises? If so, I'd like to argue for you to keep it the way it is.

Y.batch is very similar to the API I wrote for Y.Async.runAll which I have really enjoyed using. It's true that

Y.batch(fn);

is not much different to

Y.batch(Y.Promise(fn));

but when your code starts to include dozens of promises, something like

Y.batch(function (resolve) {
    Y.batch(someFn, someFn, someFn, someFn, someFn).then(resolve);
}, function (resolve) {
    Y.batch(someFn, someFn, someFn, someFn, someFn).then(resolve);
}, function (resolve, reject) {
    Y.batch(someFn, someFn, someFn, someFn, someFn).then(resolve, reject);
}, function (resolve, reject) {
    Y.batch(someFn, someFn, someFn, someFn, someFn).then(resolve, reject);
}, anotherFn, anotherFn, anotherFn);

and if you have many such bits of code within a project, you really start to appreciate not having to repeatedly type new Y.Promise(...). Every repetition of new Y.Promise(fn) is replaced by the one line typeof fn === 'function' ? new Y.Promise(fn) : fn and I think that's a win.

I would even go so far as to say, you should allow new Y.Promise(otherPromise). If otherPromise is an instance of Y.Promise, just return otherPromise. If otherPromise is some other Promises A+ compatible promise object it should return a new promise that would resolve when the other promise resolves, reject when the other promise rejects, etc. This way, all new API's could accept either functions or any promises and they could be normalized to a Y.Promise by simply calling new Y.Promise(argument)

@juandopazo
Copy link
Member

would even go so far as to say, you should allow new Y.Promise(otherPromise).

That's what Y.when is for. Y.when(5) will return a new promise fulfilled with the value 5. Y.when(promise) will just return the promise.

Currently Y.batch takes 3 different types of arguments: promises, functions and values. Promises and values (that are not functions) are passed through Y.when. So Y.batch(5) is equivalent to Y.batch(Y.when(5)). I didn't expect users to create that many promises in place. I expected them to do stuff like Y.batch(Y.io.js('foo.js'), Y.io.css('foo.css')). I know functions as values in this case is a rarity, but I want to simplify as much as possible around promises because they already have a substantial mental cost.

@solmsted
Copy link
Contributor

I completely looked over the call to Y.when.

Okay, I see how that makes more sense, assuming you're working with other API's that return promises. Since YUI and Node.js don't currently have APIs that return promises, I'm very accustomed to using anonymous functions to wrap the async functionality everywhere. As YUI's use of promises grows, I'll be okay with Y.batch accepting promises or values. I'll always have gallery-async where I can hack in my own sugar.

@lsmith
Copy link
Contributor Author

lsmith commented Jan 14, 2013

@solmsted another two points worth mentioning:

  1. Passing a function to Y.batch to get its promise wrapping magic is a convenience for what should be the uncommon case, but if Y.batch is used with a rash of anonymous functions, there's something wrong with the code. Most or all of those functions should be extracted out to methods, in which case, the function bodies can accept values and return promises.
  2. a function is a value, so treating functions differently makes it difficult to promise wrap functions as values rather than promise executors. The inconvenience is roughly equivalent, between Y.batch({wrapped:functionAsValue}, functionAsExecutor) and Y.batch(functionAsValue, Y.Promise(functionAsExecutor)), so it's more a matter of style and api-of-least-surprise.

I like the convenience, but it sacrifices a reasonable use case and promotes anon-fn hackery over more maintainable styles.

@solmsted
Copy link
Contributor

@lsmith Yes I completely agree with your 2nd point. I failed to notice the call to Y.when and never considered that simple values would be passed to Y.batch. So I'm fine with Y.batch treating functions as values.

It doesn't seem necessary to add noise here but sometime in the future I'd like to resume a conversation about methods returning a promise vs anonymous callback functions.

@lsmith
Copy link
Contributor Author

lsmith commented Feb 12, 2013

Closing this in favor of #445

@lsmith lsmith closed this Feb 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants