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

[Enhancement]: automatically parse all options as string #5396

Closed
milanholemans opened this issue Aug 12, 2023 · 4 comments
Closed

[Enhancement]: automatically parse all options as string #5396

milanholemans opened this issue Aug 12, 2023 · 4 comments

Comments

@milanholemans
Copy link
Contributor

milanholemans commented Aug 12, 2023

We use Minimist to parse options to typescript variables. By default, Minimist parses all options what he thinks is best. abc will be parsed to a string, 123 to a number etc.

This can lead to unwanted behaviors. When we type an option as string in our interfaces, we expect this property to be a string in our code as well. However it's possible that a user passes 123 as value to a string typed option. In this case, Minimist will parse it as a number. Because we type it as string, all our validators fail because you can't do a .toLowerCase() function on a number for example. When this happens, the user will see a very nasty error in the console which is absolutely no help.

The good thing is, we can tell Minimist the type of options, we can tell which options are a boolean, and which are a string. The best way to tackle this issue is to type all our options as string or boolean in Minimist. But this would lead to things like the code sample below:

#initTypes(): void {
this.types.string.push('appName', 'roleName', 'userId', 'userName', 'groupId', 'groupName', 'environmentName');
}

This is very much "useless" work. A better approach would be to type all options as string by default in Minimist, and use the #initTypes function to define the options that are no strings (numbers, boolean).

Example

interface Options {
   name?: string;
   displayName?: string;
   id?: string;
   mail?: string;
   amount?: number;
   sendMail?: boolean;
}

To be sure that all options are correctly typed, we now have to tell this to Minimist by doing:

#initTypes(): void { 
   this.types.string.push('name', 'displayName', 'id', 'mail');
   this.types.booleans.push('sendMail');
 } 

A better approach would be to parse all options as string by default and add:

#initTypes(): void { 
   this.types.numbers.push('amount');
   this.types.booleans.push('sendMail');
 } 
@milanholemans milanholemans added enhancement needs peer review Needs second pair of eyes to review the spec or PR labels Aug 12, 2023
@waldekmastykarz
Copy link
Member

Let's also not forget option aliases/shorts which from minimist point of view are considered separate options

@waldekmastykarz waldekmastykarz added needs design and removed needs peer review Needs second pair of eyes to review the spec or PR labels Aug 13, 2023
@milanholemans
Copy link
Contributor Author

@pnp/cli-for-microsoft-365-maintainers is this ready to be opened?

@Jwaegebaert
Copy link
Contributor

LGTM!

@milanholemans
Copy link
Contributor Author

We are currently doing some research to move to yargs-parser with ZOD, this issue is no longer needed.

@milanholemans milanholemans closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2024
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

4 participants