[Request] Improve cancellation API about clack HOT 7 OPEN

I'm sure frontend-Twitter has something to say on the matter of signals 😜

from clack.

Why not just have the Promises for collecting an input reject if cancelled, and resolve if successfully finished?
AbortControllers/AbortSignals are maybe the more correct way to model this, but also additional complexity & boilerplate for something that seems like a pretty common & simple use case.

from clack.

I had started writing a reply in #68, so a little out of context, but mostly a reply to @cpreston321:

I am not against the simplicity and consistency, but again my goal was to create a wrapper utility function within prompts to add grouping and also by solving the case for cancel for multiple prompts.

Right, I had similar thoughts but you beat me to it. But this has now been solved and would still be solved by the proposed solution, no?

Then prompts group would mimic what core would do in the future by exposing a (callback function or a hook) to prompts that would then carry over to child package to make a easy integration in the future.

Here I'm not following. I've heard you mention moving parts of the cancel logic into core a couple of times, but not seen any concrete plan or sign-off from @natemoo-re on this?

We had similar request here on issues #37 and #54 that describe that they wanted to see the some type of onCancel callback within a prompt.

I read that they don't want to repeat themselves with handling cancel logic for each prompt , which group solves. I don't think specifically settling on an onCancel callback is definite and the best way to do so.

Keeping it simple is also subjective, some people might find it easier to just work with the callback functions but then again in some cases might find it easier using the if(isCancel(result)).. I find the callback function is more intuitive at first glance and DX IMO but again that is just me 🤷🏼‍♂️ .

Yeah, for sure. Most things are. I don't find it more intuitive, nor better DX. I'll try to explain why in more detail:

Apart from being inconsistent with the API of individual prompts*, I prefer having one outlet. I find it confusing and potentially hard to reason about mixing Promises and callbacks. I call an async function and awaits its results. But it might not resolve (or will it?) because we also provide a callback, which might be invoked (or will it?).

* having the API of individual prompts follow the group API might be a potential solution, whatever we settle on in the end, to this problem

Can't check previous results filled out of the group. (only returns symbol)
/e.g User has 10 prompts within a group. They filled out all prompts to step 8 then cancel. I want my telemetry data to be submitted of all the results to see what step they canceled on to improve prompt in the future/
e.g /Developer wants the user to detour to another prompt based on if a prompt was filled out within the group and matches a certain value but canceled/
e.g Developer wants to display clean exit message based on certain results filled out.

Thanks for the examples. Always helpful with real-world use cases. I don't know about the last one, but the others sound legit.

OK, so I'm totally with you that useful data is lost with the cancel symbol. While getting rid of onCancel and the extra options object, I can think of two alternative approaches which would still provide the necessary information:

1. Take inspiration from the functional programming world and use a poor man's Either monad. I.e. simply replace cancel with an enhanced Error object which would provide the necessary details: cancelled step and partial results. So the return type would be Results | CancelError. We could either still provide a corresponding isCancelError() function or promote usage of instanceof CancelError.
2. Take inspiration from SWR and return an object which would hold either data/results or an error. E.g.

It would look something along the lines of:

const results = await p.group({ ... });

if (isCancelError(results)) {
  cancel("Operation cancelled.");
  process.exit(0);
}

// or

const { data/results, error } = await p.group({ ... });

if (error instanceof CancelError) {
  cancel("Operation cancelled.");
  process.exit(0);
}

Hopefully we can come up with something that work in both cases and doesn't have to be one way or another.

Maybe I'm reading you wrong, but I think we should be opinionated and not provide multiple ways to handle this.

from clack.

Seems like AbortController is designed specifically for this use case, so maybe there's some inspiration to take from the signal pattern?

I'm not sure I understand at what level you suggest signals are to be introduced? Should the user provide it?

AFAIK, signals are a way for the consumer to cancel an async function. But that wouldn't be the case here, would it?

from clack.

Did I kill the discussion...?

Anyway, sort of related to what I talk about above: https://twitter.com/mattpocockuk/status/1633064377518628866

from clack.

I would like to share my two cents

npm consola package uses this library. While attempting to use the 'isCancel' api I found myself frustrated why it wouldn't work. Turns out that unjs/consola has bundled their build for distribution (?? I don't get it but maybe they want to support people not using bundlers). Because of this, symbol initialization is done in two separate instances thus leading to the symbols not being equal.

Thus, I think it is better if it does not rely on symbols. I feel like this is an instance where symbols are used when they shouldn't be.

from clack.

One solution would be using Symbol.for to allow reusable global symbols, this situation could even happen if two (hoisted) versions of clack exist as well.

from clack.