tc39 / proposal-cancelable-promises

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

CancelReason or not #15

Closed domenic closed 8 years ago

domenic commented 8 years ago

In TC39 the consensus was that we would not have a special CancelReason class, and just do e.g. throw cancel "a string". (Except not using throw cancel; see #12.) @erights was one of the big proponents of this.

In https://github.com/domenic/cancelable-promise/issues/12#issuecomment-223720955 it seems like @benjamingr would like an explanation so I created this thread to help him with that. In particular he was wanting a .stack property.

One clear reason for not having a stack property is that stack traces are usually inspected post-mortem (either as your program crashes, or in telemetry sent via window.onerror). As cancelation at the top level doesn't kill your program, but instead disappear into the ether, there's no point at which inspecting stacks actually makes sense. If you're debugging, of course your debugger will show stack frames, but there's no need for tracking and storing and serializing a public stack property.

benjamingr commented 8 years ago

My argument is mainly that we don't really understand what we'd want from tooling or inspections in the future. I think it's entirely possible that a window.oncancelled hook could be added eventually at some point - perhaps for telemetry.

If people throw cancel strings then we remove the possibility of adding any form of context to cancellations - it means that if I log them in a catch cancel or do something with them I can't figure out where they originated. If we don't provide a standard CancelReason or something similar people will most likely use strings.

If we're making a point about not caring what the cancellation reason is - we might as well make it throw cancel; (rather than with a value).

domenic commented 8 years ago

If we're making a point about not caring what the cancellation reason is - we might as well make it throw cancel; (rather than with a value).

Yes, that was something that was also brought up in committee as a pretty good idea, although it wasn't discussed very much.

benjamingr commented 8 years ago

@erights - would throw cancel; (no string) be acceptable to you?

ljharb commented 8 years ago

If you can't provide a syntactic cancellation value, does that mean that Promise#catchCancel callbacks (or whatever they end up being called) would not receive a value either, and Promise.cancel would not take one?

NekR commented 8 years ago

@erights - would throw cancel; (no string) be acceptable to you?

const cancel = new Error('123');

if (wrong) {
  throw cancel;
}
domenic commented 8 years ago

Nobody was proposing throw cancel; only. They were proposing that the syntactic value be optional, since it's not important much of the time. Both throw cancel; and throw cancel objOrStringOrWhatever; would work.

NekR commented 8 years ago

Okay, just misunderstanding because you still always use throw cancel.

benjamingr commented 8 years ago

No, I was actually in fact proposing throw cancel; (without a value).

Either it's meaningful (so it might need a context) or it is not (at which point, why cancel with a value?).

benjamingr commented 8 years ago

@ljharb

If you can't provide a syntactic cancellation value, does that mean that Promise#catchCancel callbacks (or whatever they end up being called) would not receive a value either, and Promise.cancel would not take one?

Yes.

domenic commented 8 years ago

The committee thinks it can be useful in some situations but is not mandatory, thus it will be syntactically optional.

benjamingr commented 8 years ago

The committee thinks it can be useful in some situations but is not mandatory,

What are some situations where it can be useful?

domenic commented 8 years ago

For example, if you can be canceled for more than one reason, it is useful to be able to distinguish.

benjamingr commented 8 years ago

For example, if you can be canceled for more than one reason, it is useful to be able to distinguish.

Why? Why is it useful to distinguish?

domenic commented 8 years ago

Because you might take two actions in response to the two different reasons... Please use a little imagination instead of just asking why repeatedly.

benjamingr commented 8 years ago

Domenic, I am not being hard or unimaginative intentionally.

I genuinely cannot see a use case where the I would care about why something was cancelled nor am I sure why I would handle it. I looked for compelling use cases in other languages but:

So in every case I've found cancellation in a language it did not take a value. Now, it's entirely possible I'm missing something obvious here - but I'm not so sure I'm under that impression anymore.

annevk commented 8 years ago

Would it not be useful to record such reasons? E.g., logging the variety of reasons networking is canceled. I'd assume that as with exceptions in JavaScript it generally doesn't really matter, but it might help occasionally.

benjamingr commented 8 years ago

@annevk tooling can have privileged access (for example to stack traces) that use code doesn't have. I agree collection sounds useful but I can't really justify to myself why.

annevk commented 8 years ago

@benjamingr whenever folks talk about "tooling" they seem to forget or omit to mention that remote debugging is a thing.

ljharb commented 8 years ago

+1 on that - anything that requires the debugger, and won't work via JS API, is useless to almost all forms of headless or remote instrumentation.

benjamingr commented 8 years ago

@annevk ok, any motivating an example on how I would do it? Would you expect something like:

throw cancel "debounced"
annevk commented 8 years ago

I'm not sure I understand the question. I have no preference on syntax.

benjamingr commented 8 years ago

@annevk I'm trying to collect use cases for the cancel reason - basically to motivate the choice of either:

annevk commented 8 years ago

I see. For networking there might be a variety of reasons you cancel a request that you might want to signal somehow, "bytes are arriving too slow", "user is no longer interested", "out of storage space". These could conceivably also lead to different UX.

benjamingr commented 8 years ago

@annevk It would be great to get a concrete use case where the UX would be different based on the cancellation reason.

annevk commented 8 years ago

I mean, given the reasons I gave, it seems pretty clear that if you run out of storage and therefore cancel some podcast download, you'd tell the user that's what happens and that they might want to clear some stuff before asking again. Similarly, if the bytes are not arriving quickly enough, you'd tell the user that the network conditions are not good enough to get something transferred. When the user expresses disinterest, it's likely that you don't show anything or maybe popup some alternative activities.

benjamingr commented 8 years ago

I think no storage left should be an exception and not cancellation, but let's look at a network scenario.

So basically let's take a scenario where I have an app store and I'm downloading updates:

async function update(app, token) {
   var newCode = await fetch(url, token).then(x => x.text());
   await store(app, newCode, token);
}

async function updateAll(token) {
   await Promise.all(apps.map(x => update(x, token));
}

var token = new CancelToken(cancel => {
  networkStatusChanged.subscribe(e => {
    if(e.isConnectionMetered) cancel("Downloading on a metered connection is unsupported");
    else if(e.isPublicNetwork) cancel("Downloading over a non-home network is not supported");
    // also need to obtain new token here.
  });
});

updateAllButton.clicks.subscribe(async e => {
  try {
     await update(token);
     modal("Update Successful");
  } catch cancel (e) {
     modal(e);
  }
});
annevk commented 8 years ago

I think no storage left should be an exception and not cancellation

Sure, disk storage throws an exception, upon which you cancel the network thing going on that was feeding bits to the disk.

benjamingr commented 8 years ago

Right, but then the error message comes from the disk storage throwing an exception - the cancellation is just a side effect you're doing and not the cause of the UI change.

matthewwithanm commented 8 years ago

If it's useful to know the class of reason ("timeout"), wouldn't it also be useful to know other data about it (the duration of the timeout)?

domenic commented 8 years ago

New Cancel class is now necessary to the design so the original issue discussed here is over.