clack icon indicating copy to clipboard operation
clack copied to clipboard

[Feature Request] hooks 🪝

Open cpreston321 opened this issue 3 years ago • 11 comments

Pain Point

I was trying to find a good way of getting a of if(isCancel(res)) in favor for hooks that can be wrapped for a better DX.

If the developer had more control over the hooks it can be more easily customized, intercept events and more.

"clark:input:cancel": () => {//... code here for exit }

I have a working POC on my local computer but I first wanted to see if it's viable to do so.

Additionally you could have other hooks for "clark:init", "clark:input:submit" and pass or modify additional parameters if need be or in the future create custom ways for users to create there own "preset" of the theme using clack.

Implementation ways

  • could do it a custom implementation
  • use unjs/hookable (current working implementation).

Let me know what you think. @natemoo-re.

Thanks, CP

cpreston321 avatar Feb 14 '23 02:02 cpreston321

The only downside is, that it will increase size of module. Which is the only thing I can think of.

cpreston321 avatar Feb 14 '23 02:02 cpreston321

What it would look like

const defaultHooks = {
    'clack:input:cancel': () => {
        cancel("Operation cancelled.");
        process.exit(0);
    }
  }
  
  const dir = await text({
    message: "Where should we create your project?",
    placeholder: "./sparkling-slop",
    hooks: defaultHooks
  });

cpreston321 avatar Feb 14 '23 02:02 cpreston321

I'd be on board with something like this. I don't think we'd need to pull in unjs/hookable since the base Prompt class is already an EventEmitter and internally emits these events. We just need to expose an API to access them.

The other thing I'm considering is if we should get rid of returning Symbol('clack:cancel') entirely and only have an onCancel hook? Would remove the need for the type guard but IMO events are harder to work with since you'd need to bubble up the cancellation to the main event loop somehow (throw / catch, tracking cancellation in the parent scope, etc)

natemoo-re avatar Feb 14 '23 14:02 natemoo-re

@natemoo-re The only reason I included unjs/hookable was in favor on types and creating of new hooks so if someone created a wrapper like what you did for @clack/prompts they can expose those hooks through there package and at the core package you would have your default hooks that you then intercept. I started to think about it more, for each input you will have to do an either onCancel which made me think of some type of global settings wrapped around multiple prompts to normalize some settings but maybe it might need some more thought. 🤔💭 I will have to think of some other ideas.

I think for now onCancel callback could work to get rid of the if(isCancel())

Additionally I created this concept for prompts where you could definePromptGroup() & definePrompt() in short would wrap all yours prompts with auto typed and options that match accordingly.

Functions

export interface PromptTypes {
  text: TextOptions;
  confirm: ConfirmOptions;
  select: SelectOptions<any, any>;
  multiselect: SelectOptions<any, any>;
};

export interface ClackPromptOpts<T extends keyof PromptTypes> {
  /**
   * The name of the prompt.
   * 
   * @example `text|confirm|select|multiselect`
   */
  name: string;

  /**
   * The options of the prompt.
   */
  options?: PromptTypes[T]
}

export interface ClackPromptsGroupOpts<T extends keyof PromptTypes> {
  /**
   * The name of the group. This is used to identify the group when
   * the prompts are submitted.
   */
  name: string;

  /**
   * The prompts that are in the group that 
   * will return a big result object.
   */
  prompts: ClackPromptOpts<T>[];
};

/**
 * `definePrompt` - Define a prompt with options
 * 
 * @param {String} type - which prompt type to use?
 * @param {ClackPromptOpts} options - options for the prompt
 */
export function definePrompt<T extends keyof PromptTypes>(type: T, options: ClackPromptOpts<T>) {
  return {
    type,
    ...options
  };
}

/**
 * `definePromptGroup` - Define a group of prompts with options
 * 
 * @example
 * ```ts
 * import { definePromptGroup, definePrompt } from '@clack/prompts';
 * 
 * const prompts = definePromptGroup({
 *   name: 'my-group',
 *   prompts: [
 *    definePrompt('text', {
 *     name: 'name',
 *     options: {
 *       message: 'What is your name?'
 *     }
 *    })
 *   ]
 * })
 * ```
 */
export async function definePromptGroup<T extends keyof PromptTypes>(opts: ClackPromptsGroupOpts<T>) {
  const { name, prompts } = opts;
  const results = {
    [name]: {}
  };

  const mapPromptsByType = {
    text,
    multiselect,
    select,
    confirm
  }
  
  for(const prompt of prompts) {
    const opts = prompt.options || {}
    // @ts-expect-error - Dynamic key lookup
    const result = await mapPromptsByType[prompt.type](opts);
    
    results[name] = {
      ...results[name],
      [prompt.name]: result
    }
  }

  return results
}

Conclusion

This could potential get rid setting common options that can be set at the group level like onCancel and could define prompts with a single function without many imports.

In the meantime I will try to think of other ideas that maybe could be possible.

Thanks, CP

cpreston321 avatar Feb 14 '23 14:02 cpreston321

Curious if the group PR you landed solves this use case or if there's still more you'd like to see here!

natemoo-re avatar Feb 17 '23 11:02 natemoo-re

@natemoo-re As I think this temporarily solves for clack/prompts it will not be solved at the core package which I do think, it would better placement for these hooks still or open to just making a callback within core.

This makes it easier for people designing there own CLI tool with clack/core.

cpreston321 avatar Feb 17 '23 12:02 cpreston321

I'm curious what use-cases you're trying to support by adding hooks? I think the motivation of the proposal is a little vague for others to discuss alternatives.

And what other hooks would be added in the future? It might not make sense if the only hook is cancel.

I agree that using a hook and throwing an error will definitely add more complexity as each call would potentially have to be wrapped in a try-catch and add more scopes.

This doesn't necessarily solve the problem but just as an API idea: instead of a hooks property, I wonder if the Promise can be returned with extra properties (like execa) to double as an event emitter:

const confirming = confirm({ message: 'Delete files?' })

confirming.on('cancel', () => { process.exit(1) });

const confirmed = await confirming;
if (confirmed) {
   // ....
}


BTW I actually really liked the simplicity of being able to check isCancel in the same scope as the prompt.

The only problem is I was unfamiliar with isCancel at first, and the fact that a Symbols are truthy was misleading:

const confirmed = await confirm({ message: 'Delete files?' })
if (confirmed) {
   // ....
}

Once I saw the return types I understood, so I think the DX is fine.

privatenumber avatar Feb 17 '23 23:02 privatenumber

@privatenumber Sorry it was written up poorly and I apologize for that.

To be clear this isn't just for @clack/prompts as it is one of the use cases, I was thinking the hooks could exist within @clack/core. You could possible add additional hooks to expose from the core. These are just some examples I came up with, but there could be ones that make way more since.

  • submit - after a prompt has been submitted.
  • keypress - when the keypress is typed. (to then expose certain events to the developer maybe not the end-user)
  • render - when the prompt renders.
  • close - when the prompt closes.
  • cancel when the user cancels out.
  • etc..

Just like @natemoo-re said @clack/prompts is just a wrapper of core to make it look prettier but by exposing certain hooks this could not only help other developers wrap there own prompt. Also could be use create new hooks by the developer for them to expose to there developers/users if need be.


The isCancel is not bad in my opinion either. I do think it's easy once you understand it too but lets say you wanted to write 5 - 10 prompts? I would want to say I wouldn’t want to write this every time IMO.

if (isCancel(promptName)) {
 cancel('Operation Canceled')
 process.exit(0)
}

I do think this is quite unnecessary but maybe you disagree which is totally fine. Again that is just my opinion.

Conclusion

Furthermore, I do think hooks could be the solve to most situations within core and other packages using it. Maybe hooks isn't the right thing for it maybe just exposing events or some other method that could potentially be the solve for.

Again, if you have any other idea's I wouldn't be opposed to it all. Idea's are very welcome. I appreciate your feedback! I like to see all sides to try to find the best path forward.

Thanks, CP

cpreston321 avatar Feb 18 '23 02:02 cpreston321

Thanks for elaborating, CP

I do think hooks could be the solve to most situations within core and other packages using it. Maybe hooks isn't the right thing for it maybe just exposing events or some other method that could potentially be the solve for.

Is there a specific example you can bring up where hooks would be an improvement?

Having a concrete problem would be make the discussion much more productive because we can tackle it together. It also ensures extra code isn't added only to support hypothetical scenarios.


RE: Your event list

Perhaps the use-cases are primarily for devs using core (but in those cases, can't you just extend the classes to tap into the hooks?). But as a user, I can only imagine use-cases for cancel or keypress. I think keypress can be pretty useful for eager-loading or instant-searching in a search text box.

I'm not sure if I understand the submit event considering forms don't actually exist; isn't this just the promise resolution? Also how are they different from close?

On the Web, the render hook is definitely useful. But in a terminal UI where there's no DOM, I'm not sure I would find it useful.

RE: Your code example

The current API offers flexibility because you can have different cleanup logic right underneath each prompt. And if the cleanup logic is repetitive, it could be abstracted out into a function or assertion. I'm skeptical if the hooks API would be able to match it in terms of flexibility (if isCancel were to be replaced).

I'm also curious how the code would be more concise with hooks. Wouldn't you have to wrap the entire block in a try-catch with a singular cleanup logic?

privatenumber avatar Feb 18 '23 03:02 privatenumber

BTW I actually really liked the simplicity of being able to check isCancel in the same scope as the prompt.

I'm behind this. I think the indirection hooks and/or events introduce are not worth it in this case.

Even more so, I still think group should follow the API of individual prompts and return the cancel symbol. No LOCs saved by the extra options object vs. one guard. I'll submit a proposal at some point.


IMO, since the original problem of consolidating the cancel logic is solved by group, I'd suggest we hold off on this until concrete use cases present themselves.

ulken avatar Feb 18 '23 17:02 ulken