Throw error when SP URL is null

[milanholemans]

While testing a command I used a variable to store my site URL so it didn't clutter my command args too much. However, I was getting a strange error as shown below:

This error was due to the fact that I made a typo and --webUrl was null (because the variable I specified didn't exist).

This is because the validator we are using is as follows:

cli-microsoft365/src/utils/validation.ts

Lines 349 to 360 in 3152fc7

	isValidSharePointUrl(url: string): boolean | string {
	if (!url) {
	return false;
	}
	if (url.indexOf('https://') !== 0) {
	return `${url} is not a valid SharePoint Online site URL`;
	}
	else {
	return true;
	}
	},

In the command (and other commands) we validate it like this:

cli-microsoft365/src/m365/spo/commands/file/file-add.ts

Lines 139 to 142 in 3152fc7

	const isValidSharePointUrl: boolean | string = validation.isValidSharePointUrl(args.options.webUrl);
	if (isValidSharePointUrl !== true) {
	return isValidSharePointUrl;
	}

Shouldn't we output an error message when the value is null? The current error Error: false doesn't help at all.

[Adam-it]

yes we should 😉.
also we should double-check this behavior in other shells. in bash probably this would be just an empty string. I don't know about other.
luckily we usually use isValidSharePointUrl for this validation so I hope this would only require us to do some refactoring in this single place

[Adam-it]

I even got something worse 😉

[milanholemans]

I even got something worse 😉

Yes, that's the importance of #initTypes right now. We should type all options as much as possible. If we don't, Minimist will decide what type a variable is. When you pass --webUrl null, it translates to --webUrl which is a flag. In that case, it will get the value true, and .indexOf is not a function of a boolean value. We have not paid enough attention to this in the past.

[Adam-it]

awww right.

Bummer, what if we create an issue to fix this problem globally and add the missing #initTypes?

[milanholemans]

Bummer, what if we create an issue to fix this problem globally and add the missing #initTypes?

It's a lot of work since this applies to every single option in every single command. The same thing will happen when you run entra m365group get --id $iDoNotExist.
I know this is a known issue, and I try to fix this gradually in every command I work on/review.

The good news is, with the implementation of ZOD, this will all be fixed because every option will be typed automatically. So I suggest that we just deal with it right now. Otherwise, we have to fix this in 90% of all commands right now, and we still have to implement ZOD in every command, which will fix it anyway. So that's kind of (a lot of) double work.

[Saurabh7019]

Can I work on it?