I went through the documentation of Zod. Certainly looks interesting.

There are some areas where I immediately see how we can apply it, for example: defining different option object shapes, depending on a certain key. (For example the authType of the login command, which could solve the optional required thing)

Other areas do not immediately seem obvious, like optionSets.

It would solve the flag problem which I noticed the other day. Using zod and coercion of primitives.

It would be great to do a PoC to see it in action and explore its limitations if any in our scenario. There was also an alternative to zod which was way smaller which we could also look into, but I can't recall its name. Will need to try and find it back

Might've been valibot

There's a list of competitors on their site. Valibot isn't one of 'em...

seems like another huge refactor is on the way 😅.
good suggestion. Let's do some research 📚💪

We agreed that I'll do a POC so that we have something to look at and see how it meets our needs

OK, I've done some work, and here's what I've got.

Using zod, we can define a schema that allows us to replace initOptions, initValidators and initOptionSets. Here's how a sample schema looks like for the login command:

const globalOptions = z.object({
  query: z.string().optional(),
  output: z.enum(['csv', 'json', 'md', 'text', 'none']).optional(),
  debug: z.boolean().optional().default(false),
  verbose: z.boolean().optional().default(false)
});

const options = globalOptions.extend({
  authType: (z.enum(['certificate', 'deviceCode', 'password', 'identity', 'browser', 'secret']).optional().default('deviceCode') as unknown as ZodAliasType).alias('t'),
  cloud: z.nativeEnum(CloudType).optional().default(CloudType.Public),
  userName: z.string().optional(),
  password: z.string().optional(),
  certificateFile: z.string().optional()
    // single property validator
    .refine(filePath => !filePath || fs.existsSync(filePath), filePath => ({
      message: `Certificate file ${filePath} does not exist`
    })),
  certificateBase64Encoded: z.string().optional(),
  thumbprint: z.string().optional(),
  appId: z.string().optional(),
  tenant: z.string().optional(),
  secret: z.string().optional()
})
  // option sets
  .refine(options => options.authType !== 'password' || options.userName, {
    message: 'Username is required when using password authentication'
  })
  .refine(options => options.authType !== 'password' || options.password, {
    message: 'Password is required when using password authentication'
  })
  .refine(options => options.authType !== 'certificate' || !(options.certificateFile && options.certificateBase64Encoded), {
    message: 'Specify either certificateFile or certificateBase64Encoded, but not both.'
  })
  .refine(options => options.authType !== 'certificate' || options.certificateFile || options.certificateBase64Encoded, {
    message: 'Specify either certificateFile or certificateBase64Encoded'
  })
  .refine(options => options.authType !== 'secret' || options.secret, {
    message: 'Secret is required when using secret authentication'
  });

zod implements basic type validation that you can extend using the refine function. By adding refine to a property definition, you define a validator for a single value. If you want to validate multiple values, like in option sets, you add refine to the parent object.

For defining a short option, I added a custom function named alias. zod doesn't support it natively, and as far as I can see, there's no way to extend a class definition with an extra function in TypeScript, hence the extra cast.

What's cool about this approach, is that we define everything in a single place. We've got one schema that defines options, aliases, autocomplete and whether they're required or not. When the validation succeeds, we get a strongly-typed object.

zod runs its validation on an object. To convert user-provided command-line args into an object that we can validate with zod, we can use yargs-parser (we use minimist for this now). yargs-parser is very similar to minimist but offers some more control about parsing numbers, removing aliases, etc.

Next, I'll update one of the commands to see how it would look like inside the CLI. Looking forward to hearing your thoughts @pnp/cli-for-microsoft-365-maintainers

One more benefit that we get with zod, is that we can work with enums more reliably by getting design-time typesafety, eg:

// define output enum type
const OutputType = z.enum(['csv', 'json', 'md', 'text', 'none']);
type OutputType = z.infer<typeof OutputType>;

// add output to global options, make optional
export const globalOptions = z.object({
  query: z.string().optional(),
  output: OutputType.optional(),
  debug: z.boolean().optional().default(false),
  verbose: z.boolean().optional().default(false)
});

// parse user-provided command line args using yargs-parser
const output = ["--output", "json"];
const argv = parse(output);

// check what output was provided
switch (argv.output) {
  // cool! design-time typesafe enum without fiddling with strings
  case OutputType.enum.csv:
    console.log('csv');
    break;
  case OutputType.enum.json:
    console.log('json');
    break;
  case OutputType.enum.md:
    console.log('md');
    break;
  case OutputType.enum.text:
    console.log('text');
    break;
  case OutputType.enum.none:
    console.log('none');
    break;
}

For commands that allow unknown options, we extend the schema with .and(z.any()):

const options = globalOptions.extend({
  authType: (z.enum(['certificate', 'deviceCode', 'password', 'identity', 'browser', 'secret']).optional().default('deviceCode') as unknown as ZodAliasType).alias('t'),
  cloud: z.nativeEnum(CloudType).optional().default(CloudType.Public),
  userName: z.string().optional(),
  password: z.string().optional(),
  certificateFile: z.string().optional()
    .refine(filePath => !filePath || fs.existsSync(filePath), filePath => ({
      message: `Certificate file ${filePath} does not exist`
    })),
  certificateBase64Encoded: z.string().optional(),
  thumbprint: z.string().optional(),
  appId: z.string().optional(),
  tenant: z.string().optional(),
  secret: z.string().optional(),
  dummyNumber: z.number().optional(),
  dummyBoolean: z.boolean().optional(),
})
  .refine(options => options.authType !== 'password' || options.userName, {
    message: 'Username is required when using password authentication'
  })
  .refine(options => options.authType !== 'password' || options.password, {
    message: 'Password is required when using password authentication'
  })
  .refine(options => options.authType !== 'certificate' || !(options.certificateFile && options.certificateBase64Encoded), {
    message: 'Specify either certificateFile or certificateBase64Encoded, but not both.'
  })
  .refine(options => options.authType !== 'certificate' || options.certificateFile || options.certificateBase64Encoded, {
    message: 'Specify either certificateFile or certificateBase64Encoded'
  })
  .refine(options => options.authType !== 'secret' || options.secret, {
    message: 'Secret is required when using secret authentication'
  })
  // allow unknown options
  .and(z.any());

After fiddling with it some more, I found out that most likely we can't expose the whole schema incl. option set validation as one definition. globalOptions.extend() returns a ZodObject. You need a reference to it, if you want to extend the schema (ie. add more properties). As soon as you call refine() on the schema, the return type changes to ZodEffects. So, if we're to combine the schema in validation in one definition, we wouldn't be able to extend it in child commands that inherit from the definition. I'm suspect this would be blocking us because we're defining validators on different levels. I think we'll get more clarity when we try to add it to CLI codebase.

Interesting work @waldekmastykarz 👍

I've got a working setup with options definition and single-option validation being in one part and the option sets being in another. Had to change alias into a function to retain typesafety. Next stop, implementing it to one of our commands to see how it would fit our design.

One thing I like so far a lot is fewer strings and implicit definitions. Options and their types are expressed in code which gives us more robustness and design time feedback.

Here's btw the experimental code I've been fiddling with: https://github.com/waldekmastykarz/clim365-zod/blob/main/src/index.ts. It shows the option definition, but also how we can parse schema to get the necessary information (aliases and value types) to parse args with yargs-parser.

Nice research @waldekmastykarz! 👏 Looks like something we could definitely use.

A few remarks from my side:

• Are optional option sets (runsWhen) supported?
• In Minimist, we can't type something as a number, is Zod able to do this?

Are optional option sets (runsWhen) supported?

Yes, this is basically the condition you specify on a schema-wide refiner, eg.

.refine(options => options.authType !== 'password' || options.userName, {
    message: 'Username is required when using password authentication'
  })

Runs only when authType is set to password. Reading the condition takes getting used to, because you specify the condition you want to be true, but then specify the message for when the condition failed.

In Minimist, we can't type something as a number, is Zod able to do this?

We'd do this in yargs-parser, which supports it. If we wait for zod, it's too late because the data is lost by then by whoever parsed command line args.

@waldekmastykarz another question. We have very few options that take 2 types as value. Take planner task set for example.

Is this supported by ZOD? This will also be quite annoying with yargs-parser right? Worst case we always have to parse it as string.

zod allows you to define union types so in this can it could be number or enum. The tricky part would be parsing it properly using yargs-parser. Ideally, we'd exclude it from explicit types so that yargs-parser can figure out where the value is a string or a number itself (which it should do just fine). The problem is, that the solution we're looking at, we're automatically mapping zod schema to yargs-parser types and have no idea to exclude an option, so perhaps treating the value as a string ourselves, and maybe adding a transform to zod schema is the second best choice.

Awesome work on this one @waldekmastykarz! Looks very promising to use.

OK, I've got one command (login) done. Check out: waldekmastykarz@390cfda. I've also replaced minimist with yargs-parser. The current setup is so that both old and new zod-based validations work side by side. That way we can gradually replace it across all commands over time. zod-based validation is btw 3x slower than our: 0.6ms vs. 0.2ms, still the difference is imperceptible.

This is just the first step, so looking forward to hearing what you think.

I've added support for dynamically building the telemetry: waldekmastykarz@fabf164. It turns out, that we don't need to parse the zod schema. Instead, we can use the command options info, which we gather while building the project. We have this information when loading the command and to get access to it, we need to pass it from the runtime into the command. Then it's just a matter of iterating over the options and correctly recording the different properties.

Nice! That way it simplifies the command codebase even more. Looks very clean!

Just pushed one more commit to include the necessary tests for 100% coverage. I suggest that we:

1. Have a final critical look at the work so far
2. Decide if we need to do some other commands to validate the concept before we commit to it

Nice! That way it simplifies the command codebase even more. Looks very clean!

It's pretty cool to see how much we can simplify building commands. Building the schema will take getting used to, but with a few examples, we'll have the most cases covered. Looking forward to seeing it adopted across our codebase and finally removing the obsolete code. 🚀

I'll try to make some time to go through it more thoroughly. But, like mentioned before, big fan of the fluent writing style. It'll indeed take a bit of time to get used to but the read-ability and simplicity in the end will make it worth it.

I just pulled the code locally and took a deeper look at the setup. It seems like this will be a substantial refactor, especially since we'll need to update the tests too. However, the tests won’t be asynchronous anymore, which is a benefit. Here are a few observations:

• In zod.ts, we're using any for several parameter types, for example in the getParseFn function. Is there a specific reason for this?
• There's no error handling in the parseDef function. If getParseFn returns undefined, this could lead to problems.
• The names of functions like parseString, parseNumber, etc., imply they parse values, but they actually update the currentOption object. We might want to reconsider the naming here.

Just a few minor points, as the implementation overall seems very well done. Once again, great job on this!

• In zod.ts, we're using any for several parameter types, for example in the getParseFn function. Is there a specific reason for this?

Yes! This is a factory which is responsible for retrieving invoking specific parsing functions. In each function, we want to have access to the exact type it supports. But when getting the function, we need to support every type and as far as I can tell, there's no shared type across all types that we could, hence any.

• There's no error handling in the parseDef function. If getParseFn returns undefined, this could lead to problems.

If getParseFn returns undefined then the line that follows if (!parseDef) will be true and will break the loop, so it should be fine, unless you're referring to a different scenario?

• The names of functions like parseString, parseNumber, etc., imply they parse values, but they actually update the currentOption object. We might want to reconsider the naming here.

I chose for parse because these functions are parsing the schema. I'm open to suggestions if we feel there are better alternatives that more accurately reflect what these functions do.

But when getting the function, we need to support every type and as far as I can tell, there's no shared type across all types that we could, hence any.

I figured this might be the reason. We could create a typed alias for this scenario, but I'm not sure it's worth the effort.

If getParseFn returns undefined then the line that follows if (!parseDef) will be true and will break the loop, so it should be fine, unless you're referring to a different scenario?

Well, appeared to overlook that one 😅

I chose for parse because these functions are parsing the schema. I'm open to suggestions if we feel there are better alternatives that more accurately reflect what these functions do.

Good point. I considered using configureAs... but it does make the name longer, so I'm not sure if it actually makes things clearer.

With that, @pnp/cli-for-microsoft-365-maintainers are we ready to accept this PR or is there anything else we should research first?

Had one more check (I hope final) on the implementation:

• I like the fact we moved the telemetry outside of the command it seems a huge plus. BUT does it also mean now all options will be added to telemetry? Previously we decided which options were included and usually not all were there
• as for the zod implementation: I don't have a strong opinion on the fluent writing style. Maybe I am just old or good used to what we have 😉. It seems a bit similar to like we handled callbacks which we refactored to async/await 😜. Anyway, It seems to be less code so fine for me 🫡👍
• I don't mind it is a 'bit' slower. I think it is not noticeable 😉
• it seems to be clean especially the way we extend the options and define all the 'rules' in a single place

My opinion: If we want to make this step (and we want) and we see benefit in it (why reinvent the wheel right?) then lets do it! if zod implementation may work side by side to what we already have I don't see why we do such a hold up and I would open a PR with it 👍.
So to sum up:
Awesome work @waldekmastykarz, you Rock 🤩

• fine for me as of now
• let's open a PR and stop waiting
• IMO the PR should be approved by at least 2 maintainers
• let's go

• I like the fact we moved the telemetry outside of the command it seems a huge plus. BUT does it also mean now all options will be added to telemetry? Previously we decided which options were included and usually not all were there

The schema decides which properties are tracked and we're using the same logic which we're using now:

• required options are not tracked
• optional options with string and numeric values are tracked if they're set, but not what the value is
• optional enums are tracked with values

I'll get on submitting the PR. I appreciate your feedback!

A ok, perfect than 🤩😍

I've opened the PR. After we merge it, let's create an epic where we can track upgrading all commands to use zod and finally decommissioning the obsolete code.