fix(options): revive unknown-option validation as a staged warning

validateOptions built its lookup map keyed by each option's stringified
*value* rather than its name, so every entry resolved to undefined and the
consumer loop skipped all of them. No unknown option, and no malformed
option value, has been reported since. Iterate the parsed argv keys instead.

yargs reports both the dashed and the camelCase spelling of every flag, so
getCorrectOptionName now also resolves a camelCase key back to a literal
dashed declaration -- without that, the vision-ng family rejects itself --
and the loop reports each option once rather than once per spelling.

Five years of silence means an unknown flag may well be load-bearing in
somebody's script, and the value-shape checks have been dead just as long.
Both now warn by default and only fail when NS_STRICT_OPTIONS=error, so the
hard-fail can be previewed before it ships.

Commands that forward their options to another CLI cannot be validated
against this CLI's dictionary; they opt out with skipOptionsValidation.
preview, which passes raw argv to @nativescript/preview-cli, is the first.

Author: edusperoni

lib/commands/preview.ts

@@ -12,14 +12,15 @@ const PREVIEW_CLI_PACKAGE = "@nativescript/preview-cli";
 
 export class PreviewCommand implements ICommand {
 	allowedParameters: ICommandParameter[] = [];
+	skipOptionsValidation = true;
 
 	constructor(
 		private $logger: ILogger,
 		private $errors: IErrors,
 		private $projectData: IProjectData,
 		private $packageManager: IPackageManager,
 		private $childProcess: IChildProcess,
-		private $options: IOptions
+		private $options: IOptions,
 	) {}
 
 	private getPreviewCLIPath(): string {
@@ -37,7 +38,7 @@ export class PreviewCommand implements ICommand {
 				{
 					"save-dev": true,
 					"save-exact": true,
-				} as any
+				} as any,
 			);
 		}
 
@@ -78,7 +79,7 @@ export class PreviewCommand implements ICommand {
 					color.cyan("  ./node_modules/.bin/preview-cli"),
 					"",
 					"And if you are still having issues, try again - or reach out on Discord/open an issue on GitHub.",
-				].join("\n")
+				].join("\n"),
 			);
 
 			this.$errors.fail("Running preview failed.");
@@ -93,7 +94,7 @@ export class PreviewCommand implements ICommand {
 			[previewCLIBinPath, ...commandArgs],
 			{
 				stdio: "inherit",
-			}
+			},
 		);
 	}
 

lib/common/definitions/commands.d.ts

@@ -16,6 +16,13 @@ interface ICommand extends ICommandOptions {
 	dashedOptions?: IDictionary<IDashedOption>;
 	isHierarchicalCommand?: boolean;
 
+	/**
+	 * Set on commands that forward their options to another CLI: the options
+	 * they accept are not knowable from this CLI's option dictionary, so
+	 * validating them here would reject the other CLI's flags.
+	 */
+	skipOptionsValidation?: boolean;
+
 	/**
 	 * Describes the action that will be executed after the command succeeds.
 	 * @param {string[]} args Arguments passed to the command.

lib/common/services/commands-service.ts

@@ -158,7 +158,10 @@ export class CommandsService implements ICommandsService {
 		commandArguments: string[],
 	): Promise<boolean> {
 		const command = this.$injector.resolveCommand(commandName);
-		if (!command || !command.isHierarchicalCommand) {
+		if (
+			!command ||
+			(!command.isHierarchicalCommand && !command.skipOptionsValidation)
+		) {
 			const dashedOptions = command ? command.dashedOptions : null;
 			this.$options.validateOptions(dashedOptions);
 		}

lib/options.ts

@@ -68,6 +68,7 @@ export class Options {
 	constructor(
 		private $errors: IErrors,
 		private $settingsService: ISettingsService,
+		private $logger: ILogger,
 	) {
 		this.options = _.extend({}, this.commonOptions, this.globalOptions);
 		this.setArgv();
@@ -261,62 +262,98 @@ export class Options {
 		commandSpecificDashedOptions?: IDictionary<IDashedOption>,
 	): void {
 		this.setupOptions(commandSpecificDashedOptions);
-		const parsed: any = {};
-		for (const key of Object.keys(this.argv)) {
-			const optionName = `${this.argv[key]}`;
-			parsed[optionName] = this.getOptionValue(optionName);
-		}
 
-		_.each(parsed, (value: any, originalOptionName: string) => {
-			// when this.options are passed to yargs, it returns all of them and the ones that are not part of process.argv are set to undefined.
-			if (value === undefined) {
-				return;
+		const validated: string[] = [];
+		for (const originalOptionName of Object.keys(this.argv)) {
+			const optionValue = this.getOptionValue(originalOptionName);
+			// yargs reports every declared option; the ones that are not part of
+			// process.argv come back undefined.
+			if (optionValue === undefined) {
+				continue;
 			}
 
 			const optionName = this.getCorrectOptionName(originalOptionName);
 
-			if (!_.includes(this.optionsWhiteList, optionName)) {
-				if (!this.isOptionSupported(optionName)) {
-					this.$errors.failWithHelp(
-						`The option '${originalOptionName}' is not supported.`,
-					);
-				}
+			if (_.includes(this.optionsWhiteList, optionName)) {
+				continue;
+			}
 
-				const optionType = this.getOptionType(optionName),
-					optionValue = parsed[optionName];
+			// yargs emits both the dashed and the camelCase spelling of every flag.
+			// Unknown options have no declaration to normalize against, so key the
+			// dedupe off the camelCase form both spellings collapse to.
+			const dedupeKey = this.getNonDashedOptionName(optionName);
+			if (_.includes(validated, dedupeKey)) {
+				continue;
+			}
+			validated.push(dedupeKey);
 
-				if (_.isArray(optionValue) && optionType !== OptionType.Array) {
-					this.$errors.failWithHelp(
-						"The '%s' option requires a single value.",
-						originalOptionName,
-					);
-				} else if (
-					optionType === OptionType.String &&
-					helpers.isNullOrWhitespace(optionValue)
-				) {
-					this.$errors.failWithHelp(
-						"The option '%s' requires non-empty value.",
+			if (!this.isOptionSupported(optionName)) {
+				this.reportInvalidOption(
+					`The option '${this.getReportedOptionName(
 						originalOptionName,
-					);
-				} else if (
-					optionType === OptionType.Array &&
-					optionValue.length === 0
-				) {
-					this.$errors.failWithHelp(
-						`The option '${originalOptionName}' requires one or more values, separated by a space.`,
-					);
-				}
+					)}' is not supported.`,
+				);
+				continue;
 			}
-		});
+
+			const optionType = this.getOptionType(optionName);
+
+			if (_.isArray(optionValue) && optionType !== OptionType.Array) {
+				this.reportInvalidOption(
+					`The '${originalOptionName}' option requires a single value.`,
+				);
+			} else if (
+				optionType === OptionType.String &&
+				helpers.isNullOrWhitespace(optionValue)
+			) {
+				this.reportInvalidOption(
+					`The option '${originalOptionName}' requires non-empty value.`,
+				);
+			} else if (optionType === OptionType.Array && optionValue.length === 0) {
+				this.reportInvalidOption(
+					`The option '${originalOptionName}' requires one or more values, separated by a space.`,
+				);
+			}
+		}
 	}
 
-	private getCorrectOptionName(optionName: string): string {
-		const secondaryOptionName = this.getNonDashedOptionName(optionName);
-		return _.includes(this.optionNames, secondaryOptionName)
-			? secondaryOptionName
+	// yargs strips the `no-` prefix off a negated flag, so an undeclared
+	// `--no-foo` surfaces as `foo` and would otherwise be reported under a name
+	// the user never typed.
+	private getReportedOptionName(optionName: string): string {
+		return process.argv.indexOf(`--no-${optionName}`) !== -1
+			? `no-${optionName}`
 			: optionName;
 	}
 
+	private reportInvalidOption(message: string): void {
+		if (process.env.NS_STRICT_OPTIONS === "error") {
+			this.$errors.failWithHelp(message);
+			return;
+		}
+
+		this.$logger.warn(
+			`${message} This will become an error in a future release. Set NS_STRICT_OPTIONS=error to preview that behavior.`,
+		);
+	}
+
+	private getCorrectOptionName(optionName: string): string {
+		const nonDashedName = this.getNonDashedOptionName(optionName);
+		if (_.includes(this.optionNames, nonDashedName)) {
+			return nonDashedName;
+		}
+
+		// A few options are declared with a literal dashed key (vision-ng and
+		// friends). yargs still reports both spellings, so the camelCase one has
+		// to resolve back to the dashed declaration.
+		const dashedName = this.getDashedOptionName(optionName);
+		if (_.includes(this.optionNames, dashedName)) {
+			return dashedName;
+		}
+
+		return optionName;
+	}
+
 	private getOptionType(optionName: string): string {
 		const option =
 			this.options[optionName] || this.tryGetOptionByAliasName(optionName);

test/options.ts

@@ -391,6 +391,7 @@ function createOptionsWithProfileDir(defaultProfileDir?: string): IOptions {
 	const testInjector = new Yok();
 	testInjector.register("errors", {});
 	testInjector.register("staticConfig", {});
+	testInjector.register("logger", { warn: (): void => undefined });
 	let valuePassedToSetSettings: string;
 	testInjector.register("settingsService", {
 		setSettings: (settings: IConfigurationSettings): any => {