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

Draft A #5

Open
ForbesLindesay opened this issue Dec 24, 2012 · 17 comments
Open

Draft A #5

ForbesLindesay opened this issue Dec 24, 2012 · 17 comments

Comments

@ForbesLindesay
Copy link
Member

This has lots of holes in it, I'm putting it here though, because I'm hoping that we'll start to see where the holes are and fill some of them in a little.

Progress/A+

This proposal specifies how progress is triggered and propagated in a PromisesA+ promise library.

Requirements

The then method

The then method is the same as that specified in the promises-spec except that it also takes a third argument. We'll call that third argument the onProgress.

The onProgress

  1. If the onProgress is not a function
    1. It is ignored
  2. If the onProgress is a function
    1. When progress is emitted, the onProgress is called with the ProgressValue as the first argument.
    2. If the onProgress throws an exception with a .name property equal to 'StopProgressPropagation' then the error is silenced and progress is not propagated. In all other cases, the result of the function is used as the progress value to propagate.
    3. onProgress is never called once a promise has already been fulfilled or rejected.

The progress method

The resolver has a .progress(value) method. This triggers all the onProgress methods. It returns a promise which is fulfilled with undefined once all progress methods are complete or is rejected with the first (non-StopProgressPropagation) exception thrown by the handlers, if any.

@novemberborn
Copy link

What is supposed to happen when progress is propagated, and a propagated handler throws? Is the promise returned by the first resolver dependent on the results of all propagations, or only of the handlers it directly knows about?

@novemberborn
Copy link

This draft doesn't mention anything about emitting progress after the resolver has been fulfilled/rejected. AFAIK current implementations stop emitting progress at that point.

@novemberborn
Copy link

WIP implementation at https://github.com/novemberborn/legendary/compare/progress. See examples.

@domenic
Copy link
Member

domenic commented Dec 26, 2012

Some minor nits:

  • For consistency with Promises/A+, call it onProgress, not ProgressHandler.
  • The fulfillment value of the progress method should be specified to be undefined, and the rejection reaso specified to be the rejection reason of the first progress handler to throw a non-StopProgressPropagation exception.
  • It might be nice to have some terminology for non-StopProgressPropagation exceptions. Maybe that's the new definition of "reason"?

This spec doesn't include any of our discussion on propagation.

@ForbesLindesay
Copy link
Member Author

I'm inclined to say it should depend on all the propagated progress, not just that first one, or we have to find another way to display those errors.

@ForbesLindesay
Copy link
Member Author

@domenic I know this is pretty seriously lacking, I just thought a draft (even if it is a bit vague/unfinished) might help the discussion move forwards, I'll try and improve it when I get time.

@novemberborn
Copy link

I've now implemented this draft in Legendary 0.2.0, including tests. I did reword the draft a bit, and it's hard to collaborate on that unless we can create a wiki page for it. Anyway, I'm including my version below.


Progress/A+

This proposal specifies how progress is triggered and propagated in a PromisesA+ promise library.

Requirements

The then method

The then method is the same as that specified in the promises-spec except that it also takes a third argument. We'll call that third argument the onProgress.

  1. onProgress is an optional argument:
    1. If onProgress is not a function, it must be ignored
  2. If onProgress is a function:
    1. It must be called after progress is emitted, with the progress value as its first argument.
    2. Unless the onProgress callback throws an exception with a .name property equal to 'StopProgressPropagation', the result of the function is used as the progress value to propagate.
    3. If the onProgress callback throws an exception with a .name property equal to 'StopProgressPropagation', then the error is silenced.
    4. onProgress is never called once a promise has already been fulfilled or rejected.

The progress method

  1. The resolver has a .progress(value) method.
  2. This triggers all onProgress callbacks.
  3. It returns a promise which is fulfilled with undefined once all progress callbacks are complete,
  4. or is rejected with the first (non-StopProgressPropagation) exception thrown by the callbacks, if any.

@ForbesLindesay
Copy link
Member Author

I think we need to make it clear that onProgress callbacks can be asynchronous too. This means that the result could include rejected promises, as well as thrown exceptions.

@novemberborn
Copy link

I think we need to make it clear that onProgress callbacks can be asynchronous too. This means that the result could include rejected promises, as well as thrown exceptions.

You mean when they're propagated? I suppose that makes sense. It'd also mean that propagation waits until the returned promise fulfills.

@novemberborn
Copy link

If onProgress callbacks can return promises, and the progress is only propagated when those promises resolve, then what happens if the progress value itself is a promise? If that exact same value is returned by the callback I think it should be propagated as-is.

@ForbesLindesay
Copy link
Member Author

I wasn't really thinking we'd support a promise as the progress value, if we are then we need to think about how that's done. Is promises as progress values something anyone's using in the wild? @domenic ?

@novemberborn
Copy link

I'd be fine with not supporting that, but then we'd have to decide whether it's silently ignored or if it's an error.

@ForbesLindesay
Copy link
Member Author

Neither, I'd say it get's fully resolved. i.e. you can call resolver.progress(promise) but it's equivalent to calling:

promise.then(resolver.progress);

@novemberborn
Copy link

Works for me.

@novemberborn
Copy link

Implemented those changes in Legendary 0.3.0, with an updated spec at https://gist.github.com/4400252.

@domenic
Copy link
Member

domenic commented Dec 29, 2012

Looks pretty nice to me, although of course what exactly "propagation" means is underspecified.

I think what's most curious now is how to spec the various scenarios you can get by combining weird stuff like propagation and notification with a promise. E.g. this simple fast/slow case, but you can think of much more, especially now that you've added more tools.

I think the way to do that is to see how it behaves in Legendary (and, in the simpler cases not making use of the newer tools, in other libraries like Q, When, and jQuery). Then ask, is that how it should behave? Open an issue at this point to keep us informed. Then fix it until it behaves like you think it should. Once all those cases are behaving nicely, see if there's a set of words that describes exactly what behavior was settled on.

@novemberborn
Copy link

I think what's most curious now is how to spec the various scenarios you can get by combining weird stuff like propagation and notification with a promise. E.g. this simple fast/slow case, but you can think of much more, especially now that you've added more tools.

What do you have in mind as regards to specifying those scenarios? I've added an example of the fast/slow case and it behaves as expected: Progress is received from "fast" until it fulfills, then from "slow". Perhaps the spec could do with a section that explains how those scenarios would unfold?

I think the way to do that is to see how it behaves in Legendary (and, in the simpler cases not making use of the newer tools, in other libraries like Q, When, and jQuery). Then ask, is that how it should behave? Open an issue at this point to keep us informed. Then fix it until it behaves like you think it should. Once all those cases are behaving nicely, see if there's a set of words that describes exactly what behavior was settled on.

At this stage Legendary is just play, we'll eventually start using it at State but that won't be for a while. Ideally it'll also be the basis for promises in Dojo 2.0, but again that's some time away. My use cases have always been rather limited and this implementation supports them just fine.

I've opened #6 to collect use cases.

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

No branches or pull requests

3 participants