break(group): replace onCancel by returning cancel

[ulken]

Might be a little controversial, but I still wanted to present the case, which I find favorable.

• Consistent API with individual prompts
• Less indirection => less confusing
• Don't wrap partial results in object => simplify destruction
• Same amount of LOC
• Rewrite docs
• Simplify implementation
• Remove nonsense .catch() handler
• Remove error prone any cast

Note: this would be a breaking change, so either we would have to hold off until 1.0.0 or be less semver strict (which I think is fine before 1.0.0 is released). Might want to do a minor at least? Changeset should be updated if so.

[changeset-bot]

🦋 Changeset detected

Latest commit: e91ba5c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@clack/prompts	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[natemoo-re]

Hmm, I don't disagree that this is more consistent, but I think onCancel might allow for a bit more flexibility? The main use case I can think of is determining which step was cancelled, which isn't automatically exposed right now but could easily be passed into the callback.

Thoughts?
/cc @cpreston321

[ulken]

Hmm, I don't disagree that this is more consistent, but I think onCancel might allow for a bit more flexibility? The main use case I can think of is determining which step was cancelled, which isn't automatically exposed right now but could easily be passed into the callback.

Valid point, but I don't think the flexibility is called for. What would you do with such information?

As a user, I expect ctrl + c to terminate the program. Providing the possibility to provide an exit message is just the right flexibility, IMO. To the user it's already obvious which step they cancelled at.

I think we should try hard to keep the API minimal and simple as possible and not design for hypothetical future usages no one potentially ever uses. But that's just my take.

[cpreston321]

@ulken
/cc @natemoo-re

Follow up

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. 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.

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. 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 🤷🏼‍♂️ .

Limitations of implementation

• 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.

Conclusion

I think there might be alternative solution but as of right now I am not for sure what that looks like. Hopefully we can come up with something that work in both cases and doesn't have to be one way or another. I think both have there pros and cons.

Happy to discuss more about with more developers to get there take so then we can get a good solution for the v1 release.

Thanks,
CP

[natemoo-re]

Thanks for chiming in @cpreston321! I think I agree that the current API feels a bit odd, but I do think it solves some valid issues with the Symbol('clack:cancel') approach so I'm inclined to leave it for now.

Would love to think through some alternative APIs for cancellation. Maybe something with AbortController might make sense?

[natemoo-re]

Going to close this for now but I'd love to continue the discussion. Let's discuss in #83.