Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: usage of third party validation packages / async validations #97

Open
noxify opened this issue Feb 13, 2024 · 8 comments
Open

Comments

@noxify
Copy link
Contributor

noxify commented Feb 13, 2024

Hi,

had the following idea and want to know what you think about it ( e.g. is it in scope of this package ).

Currently the package has a simple required validation.
Which is fine if you only want to ensure that there is an input.

As developer I have based on the current implementation now the possibility to write my own validation rules.
This works for simple cases like

  1. check that the value has at least three character
  2. check that the value is between 5 and 10
  3. check that the array ( e.g. in the multiselect ) has not more than 5 values

Note: For case 3, we could also add an option to specify the limit

But I can't do more "complex" validations like

  • Use the user input to call an api endpoint to check that the value is valid ( e.g. entering an id )

Possible solutions

Using resolvers

While creating some UI for our internal apps, I often use react-hook-form for the form handling.
They have a nice implementation to support various validation frameworks.

Maybe we could adapt this approach?

Support for async validations

An alternative approach could be to support async validations but keeping the current handling as it is.

With this approach we use a "lightweight" implementation and instead of providing a resolver for every validation library, we just allow to configure async validation rules and as developer I can import my preferred library.


If I have missed something or something is unclear, please let me know.

@PierreDemailly
Copy link
Member

Hi, yes we can add support for async validators easily, however im not fan to introduce a more complex way to support this (resolvers).

We need to think how to handle async error function. Imagine you validate the id via an API then if the result is not 200, you wanna show the API error message in the prompt, it would need the validators to be reworked instead of just adding support for async/await because actually we have 2 functions: validate & error.

Maybe we should only have 1 function validate: if it returns a string then its the error message to be shown in the prompt, else (null ? undefined ? false ? what about empty string ?) we consider the input valid.

WDYT?

@noxify
Copy link
Contributor Author

noxify commented Feb 13, 2024

however im not fan to introduce a more complex way to support this (resolvers).

Fully agree with this statement - Since it was an valid approach for me, I mentioned it.

Maybe we should only have 1 function validate: if it returns a string then its the error message to be shown in the prompt, else (null ? undefined ? false ? what about empty string ?) we consider the input valid.

Sounds good for me.

What if we expect an object instead of a mixed value as return value?

Example - Validation check passed:

{
  valid: true,
  message: null
}

Example - Validation check failed ( unstyled ):

{
  valid: false,
  message: "Unstyled error message"
}

Example - Validation check failed ( styled ):

import kleur from "kleur";

{
  valid: false,
  message: kleur.red().bold("styled error message")
}

In my example I assume that we will pass the message as is to the client without doing any styling.

With this, the developer can decide how the error message is shown at the client and is not limited to a static styling.
( also we move the responsibility about the styling to the developer side )

About the responsibility - I don't think this should be a problem, because for web applications there are a lot of tools which do the same ( e.g. unstyled components like radix-ui or headless-ui ).


Maybe we have to export the kleur package, so the developer doesn't have to install it again.

// prompts/index.ts

export * as kleur from "kleur";

// custom-prompt.ts
import { question, kleur } from  "topcli/prompts"

@PierreDemailly
Copy link
Member

PierreDemailly commented Feb 13, 2024

Yes an object response seems good to me 👍

I'm not fan to export kleur, also note that we show the error in red already and i wonder if there would have conflicts if the returned message have style (need to try it to see).

In my example I assume that we will pass the message as is to the client without doing any styling.

For my own projects, I don't wanna style the error myself everytime, I wanna do less effort as possible and have all my CLI tools having the same design ^^ There is no other solution to let the red style by default.

With this, the developer can decide how the error message is shown at the client and is not limited to a static styling.
( also we move the responsibility about the styling to the developer side )

About the responsibility - I don't think this should be a problem, because for web applications there are a lot of tools which do the same ( e.g. unstyled components like radix-ui or headless-ui ).

I'm not really +1 :/
We fully handle styles/design everywhere else: the emojis before the question after it has been answered, the non-customisable Symbols constant, etc.

I'm ok to let the user customize the error message style (while red seem the most logical so it look like a 'gadget' feature) but the goal of the library is not to be an "abstract-full-customisable-prompts".

Also, if we really wanna let the users customize everything (rn we speak about the error message but it could be everything), I would instead see a "skeleton-prompt" so the user can create its own prompt based on the skeleton, so he can design it, changes colors, symbols etc then he have its perfect prompt and can eventually build his own library if he need to reuse them.

@noxify
Copy link
Contributor Author

noxify commented Feb 13, 2024

Thanks for the feedback.

Creating a custom prompt to have my own styles sounds good.

Checked the code quickly and it seems easy to create a custom prompt, since the custom prompt can extend the existing one and then the methods can be overwritten (e.g. showQuestion) - please correct me if I missed something.

Something else what needs to be clarified?

If you want, I can give it a try in the next days.

@noxify
Copy link
Contributor Author

noxify commented Feb 13, 2024

One additional question: does it make sense to move the validation message into a new line since the error message could be longer (e.g. "value must have at least 8 characters" )?

@PierreDemailly
Copy link
Member

PierreDemailly commented Feb 13, 2024

since the custom prompt can extend the existing one and then the methods can be overwritten

Yes, but it look like you're talking about you're own custom prompt (outside this library), wdym with "custom prompt" ? A skeleton/abstract prompt to be customisable ?

I was thinking about something like clack, they have @clack/prompts and @clack/core which is the "unstyled, extensible primitives for CLIs" but I think it's a bit more complex as what you're thinking ^^

One additional question: does it make sense to move the validation message into a new line since the error message could be longer (e.g. "value must have at least 8 characters" )?

I would say no

  • if we have a short message i.e "required", the UI should be better without new line imo
  • on a tight terminal, even if you move into a new line the error message may take 2 lines+
  • something like message: "\n user move to new line himself when long message" can works if we fixes the [] around the message, i think its the best thing to do if we wanna allow user to do it
    update: or find another way to make it customisable 😅

@noxify
Copy link
Contributor Author

noxify commented Feb 13, 2024

Yes, but it look like you're talking about you're own custom prompt (outside this library), wdym with "custom prompt" ? A skeleton/abstract prompt to be customisable ?

I meant something outside this library.

I was thinking about something like clack, they have @clack/prompts and @clack/core which is the "unstyled, extensible primitives for CLIs" but I think it's a bit more complex as what you're thinking ^^

That sounds like the best solution which would support all use cases but requires a full refactor and could/will result in a breaking change ( which is ok for me, maybe providing a codemod?) - I tried to find a solution which uses the existing code.

  • if we have a short message i.e "required", the UI should be better without new line imo

  • on a tight terminal, even if you move into a new line the error message may take 2 lines+

Makes sense

  • something like message: "\n user move to new line himself when long message" can works if we fixes the [] around the message, i think its the best thing to do if we wanna allow user to do it

If we update the existing required validator, we can set the brackets in validator, then we can remove it from the prompt code. This ensures that the existing tests will pass without changes.

@PierreDemailly
Copy link
Member

That sounds like the best solution which would support all use cases but requires a full refactor and could/will result in a breaking change ( which is ok for me, maybe providing a codemod?)

I guess if we introduce "small" breaking changes its ok for the value. Ideally, we can make a new library (npm workspace ?) or API and the actual prompts remains the sames even if behind the scene everything have been changed.

If we update the existing required validator, we can set the brackets in validator, then we can remove it from the prompt code. This ensures that the existing tests will pass without changes.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants