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

Don't use .promise.then for subscription #59

Closed
bergus opened this issue Oct 12, 2016 · 8 comments
Closed

Don't use .promise.then for subscription #59

bergus opened this issue Oct 12, 2016 · 8 comments

Comments

@bergus
Copy link

bergus commented Oct 12, 2016

There is no point in exposing a .promise on the token:

  • it never rejects but always fulfills, so the abstraction feels wrong anyway
  • we can't inspect a promise any better than we can inspect the token itself
  • it's not needed for token composition, which is done through custom CancelToken.race (etc.) functions anyway not Promise.race(tokens.map(t => t.promise)).then(cancel)
  • it's one extra object for the engine to instantiate
  • a token should be considered a single entity, not a composite
  • invoking token.promise.then(…) is awkward, compared to token.subscribe(…)
  • a real native promise doesn't currently allow an engine to unsubscribe callbacks (which is necessary for tokens so that they don't leak, see Allow for garbage collection of cancellation handlers on long-lived tokens #52)

I guess I could find more.
What are the advantages of using a promise as part of a token? They allowed for quick prototyping (no need to reimplement all the subscription stuff) and they demonstrated the proposed semantics in a way everyone would be familiar with. I can't think of anything else. So - good for a draft, but nothing mature enough for the language.

My suggestion therefore is to remove the getter

get promise() {
    return this.[[CancelTokenPromise]]
}

and replace it by a method

subscribe(handler) {
    return this.[[CancelTokenPromise]].then(handler)
}

Maybe the promise should be removed altogether even from the internals of CancelTokens, a [[CancelReason]] field of type Option<Cancel<T>> and a [[CancelReactions]] field of type List<Function> should suffice.

@domenic
Copy link
Member

domenic commented Oct 12, 2016

Thanks for your input, but the current direction is we're going with for cancel token subscriptions. We're not going to add another developer-facing subscription protocol to the language for single-occurrence events, even if it just wraps the promise as you describe.

@domenic domenic closed this as completed Oct 12, 2016
@bergus
Copy link
Author

bergus commented Oct 12, 2016

There is no "other" subscription protocol - it behaves the same as then. If you want, you can even call it then instead of subscribe to make tokens become thenables.
And you already got Promise.withCancelToken as another (superior, actually) method of subscribing to tokens. I would be fine with subscribe being dropped altogether as well.

TBH, closing this issue without even trying to counter my arguments looks a lot like "Yeah, I can see this is the wrong direction but it's the one we decided on". Not what I expected of TC39.

@benjamingr
Copy link

I'm in favor of keeping .promise it has been very useful when writing code with tokens. The fact it never rejects is as relevant as the fact Math.max never throws.

The engine can instantiate the object lazily if it needs to.

@bergus
Copy link
Author

bergus commented Oct 13, 2016

@benjamingr Can you post (or point to) such an example where .promise is useful? I can't imagine one where Promise.withCancelToken or a token.subscribe would not be a better fit.

@benjamingr
Copy link

I'm afk on mobile for a while and in Japan, I can post some examples on Oct 23rd but I posted some motivating examples earlier in this repo that use .promise

@bergus
Copy link
Author

bergus commented Oct 13, 2016

I might be missing something, but all the good examples I remember on this repo that used .promise used it only for a .promise.then(…) call. Looking forward for some good reason to keep the promise, thanks!

@ljharb
Copy link
Member

ljharb commented Oct 13, 2016

Any place I want to resolve another promise with it, or pass it into a function - a promise is more powerful than a callback-taking API precisely because it's composable and I don't have to have the callback ready yet.

@bergus
Copy link
Author

bergus commented Oct 13, 2016

@ljharb I don't think there are any common use cases where you'd want to pass a token's promise into a function instead of passing the token itself. If such use cases existed, making the token thenable could take care of them.
And don't forget that even subscribe is still a promise-returning API - if you desperately needed a promise, you could just call token.subscribe() (without arguments) and get one.

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

4 participants