Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Avoiding all this. #25

Closed
jan-ivar opened this issue Jun 21, 2016 · 3 comments
Closed

Avoiding all this. #25

jan-ivar opened this issue Jun 21, 2016 · 3 comments

Comments

@jan-ivar
Copy link

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
Copy link
Member

domenic commented Jun 21, 2016

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.

@domenic domenic closed this as completed Jun 21, 2016
@jan-ivar
Copy link
Author

@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
Copy link
Member

domenic commented Jun 21, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants