domenic / promises-unwrapping

The ES6 promises spec, as per September 2013 TC39 meeting
1.23k stars 94 forks source link

Can sub-classes use additional arguments for "then"? #44

Closed skaegi closed 10 years ago

skaegi commented 10 years ago

I'm writing a Promise sub-class that supports the resolver sending "progress" to listeners. e.g. my resolver accepts a third argument -- a notify function -- that is used to send "progress".

At the moment I register a listener by calling promise.then(null, null, onProgress) where onProgress is a function that expects a ProgressEvent.

Is this going to be supported or is the contract for then to only accept two arguments -- onFulfilled and onRejected?

domenic commented 10 years ago

This should work fine:

// Assume two functions `doNotification(promise, value)`
// and `addProgressListener(promise, listener)`.

class ProgressPromise extends Promise {
  constructor(resolver) {
    super((resolve, reject) => resolver(resolve, reject, x => doNotification(this, x)));
  }

  then(onFulfilled, onRejected, onProgress) {
    super(onFulfilled, onRejected);
    addProgressListener(this, onProgress);
  }
}

Is there anything in the current spec you can see that would prevent such a technique?

skaegi commented 10 years ago

Thanks @domenic, I just wanted to be sure this approach was going to be acceptable and that thinking on progress/notify hasn't changed dramatically from the thinking in https://github.com/promises-aplus/progress-spec/issues/5.

I'll write up another sub-class implementation to validate.

domenic commented 10 years ago

@skaegi well personally I think trying to tie progress to promises is not a great idea, and especially not trying to put it on the same footing as onFulfilled/onRejected as arguments to then, or as resolve/reject as arguments to the resolver. I've given some arguments in depth here why I feel that progress as part of promises is a mistake:

http://lists.w3.org/Archives/Public/www-dom/2013JulSep/0116.html (see also replies)

But anyway, it should be possible to subclass in user-space, regardless of how good or bad I personally consider the idea to be. And I think it indeed is, so, closing.

skaegi commented 10 years ago

Thanks @domenic -- that was an interesting thread. We've been heavy users of "progress" for the last couple of years and I agree with your instincts. To be honest for the way we've been using it (mostly sending ProgressEvents with no propagation) something like EventTarget might be more sensible. The only thing lost by removing onProgress from then is the propagation which as other have pointed out is a little baffling anyway. I might try an EventTargetPromise next.

Part of the reason I'm doing this work is to build up some experience for writing Promise sub-classes to see if I can help sort out what works and what doesn't. ...so for good or bad (and in this case mostly bad) here is an ES5 ProgressPromise sub-class that exercise the concepts around additional then and resolver arguments, passes the current Promises/A+ tests and matches the functionality described in Promises/A+ Progress Issues.