tc39 / proposal-cancelable-promises

Former home of the now-withdrawn cancelable promises proposal for JavaScript
Other
376 stars 29 forks source link

Avoiding all this. #25

Closed jan-ivar closed 8 years ago

jan-ivar commented 8 years ago

I think technically this works - it reads like a sound and thorough propopsal - but i wouldn't do it.

I have two big concerns with it:

Complexity

It adds a huge amount of complexity to the task of understanding basic promises, raising the bar for everyone for the benefit of a late-arriving feature desired by some. This is almost always an API mistake in my experience.

I know we all know promises have "won", but I still deal with issues like this where callbacks won't die. It's also sobering to compare stackoverflow's es6-promise tag with its 508 questions to js`'s million+ questions (unfair, I know due to scope and triage. Still, triage is imperfect, and the questions people pose about promises show many struggle with the concept).

Necessity

I don't see that we need this at all. The cancel token seems like a glorified promise to me. Here's a pattern we're using in real code (ok, tests):

/**
 * Returns a promise that resolves when `target` has raised an event with the
 * given name. Cancel the returned promise by passing in a `cancelPromise` and
 * resolve it.
 *
 * @param {object} target
 *        The target on which the event should occur.
 * @param {string} name
 *        The name of the event that should occur.
 * @param {promise} cancelPromise
 *        A promise that on resolving rejects the returned promise,
 *        so we can avoid logging results after a test has finished.
 */
function haveEvent(target, name, cancelPromise) {
  var listener;
  var p = Promise.race([
    (cancelPromise || new Promise()).then(e => Promise.reject(e)),
    new Promise(resolve => target.addEventListener(name, listener = resolve))
  ]);
  p.then(() => target.removeEventListener(name, listener));
  return p;
};

Then we use this function like this:

return haveEvent(receiver.track, "ended", wait(50000))

(See two more complex uses in the above link.)

Very pragmatic, this is garbage in, garbage out. Whatever we resolve the cancelPromise with is what it'll reject with, so keeping the rejection distinct from "real" errors becomes the user's problem, and trivial in practice.

So let me polyfill fetch with this (for illustration only):

let polyFetch = (a, b, cancelPromise) => Promise.race([
    (cancelPromise || new Promise()).then(e => Promise.reject(e)),
    fetch(a, b)
  ]);

Now obviously, this wouldn't do anything to actually cancel network traffic, it just stops waiting. But if fetch and other DOM api's adopted this cancelPromise pattern, then I see no reason it couldn't.

Obviously, this won't do everything that's proposed here, particularly, there's no cancel-path distinct from rejection, but since I haven't seen any use-cases, maybe that's good? From my viewpoint, the person cancelling will be the one responsible for the chain overall (at least the part being cancelled).

domenic commented 8 years ago

Hi Jan,

Since this issue doesn't seem to be a concrete issue with the proposal, but instead a kind of counterproposal, I'm going to close this issue since it doesn't contribute to the development happening here. If you have a counterproposal the proper process is to go through TC39.

You're welcome to continue discussing your points here if you wish, but perhaps it would be better to do so in your own counterproposal repository.

jan-ivar commented 8 years ago

@domenic If there's another venue somewhere for overall discussion, I'm happy to take it there (I was asked to bring comments here). I raised two issues I see with this proposal: complexity and necessity. Do you have use-cases to support the need for what you propose?

domenic commented 8 years ago

Yes, I have outlined them in the documents in this repository.