Fix changes to InputValidator to resolve accidental breaking changes (#5403)

* Fix changes to InputValidator to resolve accidental breaking changes

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Show changes working by removing no longer required empty props objects

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* fix type errors

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Fix async validation errors not being displayed

- And fix validation errors sometimes being displayed multiple times

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Simplify builders for validators

- Replace the boolean type params on `function inputValidator` with a
  new builder `function inputValidatorWithRequiredProps` to make the
  code easier to read

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Introduce unionizing functions for input validators

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* fix tests

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Remove RequiredProps type param

Signed-off-by: Sebastian Malton <sebastian@malton.name>

src/common/utils/type-narrowing.ts

@@ -137,6 +137,14 @@ export function hasDefiniteField<Field extends keyof T, T>(field: Field): (val:
   return (val): val is T & { [f in Field]-?: NonNullable<T[Field]> } => val[field] != null;
 }
 
+export function isPromiseSettledRejected<T>(result: PromiseSettledResult<T>): result is PromiseRejectedResult {
+  return result.status === "rejected";
+}
+
+export function isPromiseSettledFulfilled<T>(result: PromiseSettledResult<T>): result is PromiseFulfilledResult<T> {
+  return result.status === "fulfilled";
+}
+
 export function isErrnoException(error: unknown): error is NodeJS.ErrnoException {
   return isObject(error)
     && hasOptionalTypedProperty(error, "code", isString)

src/renderer/components/+extensions/install-from-input/install-from-input.tsx

@@ -14,7 +14,6 @@ import { readFileNotify } from "../read-file-notify/read-file-notify";
 import type { InstallRequest } from "../attempt-install/install-request";
 import type { ExtensionInfo } from "../attempt-install-by-info.injectable";
 import type { ExtensionInstallationStateStore } from "../../../../extensions/extension-installation-state-store/extension-installation-state-store";
-import { AsyncInputValidationError } from "../../input/input_validators";
 
 export type InstallFromInput = (input: string) => Promise<void>;
 
@@ -34,7 +33,7 @@ export const installFromInput = ({
 
     try {
       // fixme: improve error messages for non-tar-file URLs
-      if (InputValidators.isUrl.validate(input, {})) {
+      if (InputValidators.isUrl.validate(input)) {
         // install via url
         disposer = extensionInstallationStateStore.startPreInstall();
         const { promise } = downloadFile({ url: input, timeout: 10 * 60 * 1000 });
@@ -44,23 +43,19 @@ export const installFromInput = ({
       }
 
       try {
-        await InputValidators.isPath.validate(input, {});
+        await InputValidators.isPath.validate(input);
 
         // install from system path
         const fileName = path.basename(input);
 
         return await attemptInstall({ fileName, dataP: readFileNotify(input) });
       } catch (error) {
-        if (error instanceof AsyncInputValidationError) {
+        const extNameCaptures = InputValidators.isExtensionNameInstallRegex.captures(input);
-          const extNameCaptures = InputValidators.isExtensionNameInstallRegex.captures(input);
 
-          if (extNameCaptures) {
+        if (extNameCaptures) {
-            const { name, version } = extNameCaptures;
+          const { name, version } = extNameCaptures;
 
-            return await attemptInstallByInfo({ name, version });
+          return await attemptInstallByInfo({ name, version });
-          }
-        } else {
-          throw error;
         }
       }
 

src/renderer/components/+extensions/install.tsx

@@ -13,10 +13,9 @@ import { Input, InputValidators } from "../input";
 import { SubTitle } from "../layout/sub-title";
 import { TooltipPosition } from "../tooltip";
 import type { ExtensionInstallationStateStore } from "../../../extensions/extension-installation-state-store/extension-installation-state-store";
-import extensionInstallationStateStoreInjectable
+import extensionInstallationStateStoreInjectable from "../../../extensions/extension-installation-state-store/extension-installation-state-store.injectable";
-  from "../../../extensions/extension-installation-state-store/extension-installation-state-store.injectable";
 import { withInjectables } from "@ogre-tools/injectable-react";
-import { inputValidator } from "../input/input_validators";
+import { unionInputValidatorsAsync } from "../input/input_validators";
 
 export interface InstallProps {
   installPath: string;
@@ -30,18 +29,14 @@ interface Dependencies {
   extensionInstallationStateStore: ExtensionInstallationStateStore;
 }
 
-const installInputValidators = [
+const installInputValidator = unionInputValidatorsAsync(
+  {
+    message: "Invalid URL, absolute path, or extension name",
+  },
   InputValidators.isUrl,
-  InputValidators.isPath,
   InputValidators.isExtensionNameInstall,
-];
+  InputValidators.isPath,
-
+);
-const installInputValidator = inputValidator({
-  message: "Invalid URL, absolute path, or extension name",
-  validate: (value: string, props) => (
-    installInputValidators.some(({ validate }) => validate(value, props))
-  ),
-});
 
 const NonInjectedInstall: React.FC<Dependencies & InstallProps> = ({
   installPath,

src/renderer/components/input/__tests__/input_validators.test.ts

@@ -3,11 +3,86 @@
  * Licensed under MIT License. See LICENSE in root directory for more information.
  */
 
-import { isEmail, isUrl, systemName } from "../input_validators";
+import { isEmail, isUrl, systemName, unionInputValidators, unionInputValidatorsAsync } from "../input_validators";
 
 type TextValidationCase = [string, boolean];
 
 describe("input validation tests", () => {
+  describe("unionInputValidators()", () => {
+    const emailOrUrl = unionInputValidators(
+      {
+        message: "Not an email or URL",
+      },
+      isEmail,
+      isUrl,
+    );
+
+    it.each([
+      "abc@news.com",
+      "abc@news.co.uk",
+    ])("Given '%s' is a valid email, emailOrUrl matches", (input) => {
+      expect(emailOrUrl.validate(input)).toBe(true);
+    });
+
+    it.each([
+      "https://github-production-registry-package-file-4f11e5.s3.amazonaws.com/307985088/68bbbf00-309f-11eb-8457-a15e4efe9e77?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20201127%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20201127T123754Z&X-Amz-Expires=300&X-Amz-Signature=9b8167f00685a20d980224d397892195abc187cdb2934cefb79edcd7ec600f78&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=0&response-content-disposition=filename%3Dstarboard-lens-extension-0.0.1-alpha.1-npm.tgz&response-content-type=application%2Foctet-stream",
+      "http://www.google.com",
+    ])("Given '%s' is a valid url, emailOrUrl matches", (input) => {
+      expect(emailOrUrl.validate(input)).toBe(true);
+    });
+
+    it.each([
+      "hello",
+      "57",
+    ])("Given '%s' is neither a valid email nor URL, emailOrUrl does not match", (input) => {
+      expect(emailOrUrl.validate(input)).toBe(false);
+    });
+  });
+
+  describe("unionInputValidatorsAsync()", () => {
+    const emailOrUrl = unionInputValidatorsAsync(
+      {
+        message: "Not an email or URL",
+      },
+      isEmail,
+      isUrl,
+    );
+
+    it.each([
+      "abc@news.com",
+      "abc@news.co.uk",
+    ])("Given '%s' is a valid email, emailOrUrl matches", async (input) => {
+      try {
+        await emailOrUrl.validate(input);
+      } catch {
+        fail("Should not throw on valid input");
+      }
+    });
+
+    it.each([
+      "https://github-production-registry-package-file-4f11e5.s3.amazonaws.com/307985088/68bbbf00-309f-11eb-8457-a15e4efe9e77?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20201127%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20201127T123754Z&X-Amz-Expires=300&X-Amz-Signature=9b8167f00685a20d980224d397892195abc187cdb2934cefb79edcd7ec600f78&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=0&response-content-disposition=filename%3Dstarboard-lens-extension-0.0.1-alpha.1-npm.tgz&response-content-type=application%2Foctet-stream",
+      "http://www.google.com",
+    ])("Given '%s' is a valid url, emailOrUrl matches", async (input) => {
+      try {
+        await emailOrUrl.validate(input);
+      } catch {
+        fail("Should not throw on valid input");
+      }
+    });
+
+    it.each([
+      "hello",
+      "57",
+    ])("Given '%s' is neither a valid email nor URL, emailOrUrl does not match", async (input) => {
+      try {
+        await emailOrUrl.validate(input);
+        fail("Should throw on invalid input");
+      } catch {
+        // We want this to happen
+      }
+    });
+  });
+
   describe("isEmail tests", () => {
     const tests: TextValidationCase[] = [
       ["abc@news.com", true],
@@ -21,7 +96,7 @@ describe("input validation tests", () => {
     ];
 
     it.each(tests)("validate %s", (input, output) => {
-      expect(isEmail.validate(input, {})).toBe(output);
+      expect(isEmail.validate(input)).toBe(output);
     });
   });
 
@@ -38,7 +113,7 @@ describe("input validation tests", () => {
     ];
 
     it.each(cases)("validate %s", (input, output) => {
-      expect(isUrl.validate(input, {})).toBe(output);
+      expect(isUrl.validate(input)).toBe(output);
     });
   });
 
@@ -68,7 +143,7 @@ describe("input validation tests", () => {
     ];
 
     it.each(tests)("validate %s", (input, output) => {
-      expect(systemName.validate(input, {})).toBe(output);
+      expect(systemName.validate(input)).toBe(output);
     });
   });
 });

src/renderer/components/input/input.tsx

@@ -7,20 +7,37 @@ import "./input.scss";
 
 import type { DOMAttributes, InputHTMLAttributes, TextareaHTMLAttributes } from "react";
 import React from "react";
-import { autoBind, cssNames, debouncePromise, getRandId } from "../../utils";
+import { autoBind, cssNames, debouncePromise, getRandId, isPromiseSettledFulfilled } from "../../utils";
 import { Icon } from "../icon";
 import type { TooltipProps } from "../tooltip";
 import { Tooltip } from "../tooltip";
 import * as Validators from "./input_validators";
-import type { InputValidator } from "./input_validators";
+import type { InputValidator, InputValidation, InputValidationResult, SyncValidationMessage } from "./input_validators";
-import isFunction from "lodash/isFunction";
 import uniqueId from "lodash/uniqueId";
 import { debounce } from "lodash";
 
-const { conditionalValidators, ...InputValidators } = Validators;
+const {
+  conditionalValidators,
+  asyncInputValidator,
+  inputValidator,
+  isAsyncValidator,
+  unionInputValidatorsAsync,
+  ...InputValidators
+} = Validators;
 
-export { InputValidators };
+export {
-export type { InputValidator };
+  InputValidators,
+  asyncInputValidator,
+  inputValidator,
+  isAsyncValidator,
+  unionInputValidatorsAsync,
+};
+export type {
+  InputValidator,
+  InputValidation,
+  InputValidationResult,
+  SyncValidationMessage,
+};
 
 type InputElement = HTMLInputElement | HTMLTextAreaElement;
 type InputElementProps =
@@ -78,15 +95,11 @@ const defaultProps: Partial<InputProps> = {
   blurOnEnter: true,
 };
 
-function isAsyncValidator(validator: InputValidator<boolean>): validator is InputValidator<true> {
-  return typeof validator.debounce === "number";
-}
-
 export class Input extends React.Component<InputProps, State> {
   static defaultProps = defaultProps as object;
 
   public input: InputElement | null = null;
-  public validators: InputValidator<boolean>[] = [];
+  public validators: InputValidator[] = [];
 
   public state: State = {
     focused: false,
@@ -155,7 +168,7 @@ export class Input extends React.Component<InputProps, State> {
   async validate() {
     const value = this.getValue();
     let validationId = (this.validationId = ""); // reset every time for async validators
-    const asyncValidators: Promise<any>[] = [];
+    const asyncValidators: Promise<React.ReactNode>[] = [];
     const errors: React.ReactNode[] = [];
 
     // run validators
@@ -169,28 +182,15 @@ export class Input extends React.Component<InputProps, State> {
         if (!validationId) {
           this.validationId = validationId = uniqueId("validation_id_");
         }
-        asyncValidators.push(
+        asyncValidators.push((async () => {
-          validator.validate(value, this.props).then(
+          try {
-            () => null, // don't consider any valid result from promise since we interested in errors only
+            await validator.validate(value, this.props);
-            error => this.getValidatorError(value, validator) || error,
+          } catch (error) {
-          ),
+            return this.getValidatorError(value, validator) || (error instanceof Error ? error.message : String(error));
-        );
+          }
-      }
+        })());
-
+      } else if (!validator.validate(value, this.props)) {
-      const isValid = validator.validate(value, this.props);
-
-      if (isValid === false) {
         errors.push(this.getValidatorError(value, validator));
-      } else if (isValid instanceof Promise) {
-        if (!validationId) {
-          this.validationId = validationId = uniqueId("validation_id_");
-        }
-        asyncValidators.push(
-          isValid.then(
-            () => null, // don't consider any valid result from promise since we interested in errors only
-            error => this.getValidatorError(value, validator) || error,
-          ),
-        );
       }
     }
 
@@ -200,10 +200,15 @@ export class Input extends React.Component<InputProps, State> {
     // handle async validators result
     if (asyncValidators.length > 0) {
       this.setState({ validating: true, valid: false });
-      const asyncErrors = await Promise.all(asyncValidators);
+      const asyncErrors = await Promise.allSettled(asyncValidators);
 
       if (this.validationId === validationId) {
-        this.setValidation(errors.concat(...asyncErrors.filter(err => err)));
+        errors.push(...asyncErrors
+          .filter(isPromiseSettledFulfilled)
+          .map(res => res.value)
+          .filter(Boolean));
+
+        this.setValidation(errors);
       }
     }
 
@@ -218,10 +223,10 @@ export class Input extends React.Component<InputProps, State> {
     });
   }
 
-  private getValidatorError(value: string, { message }: InputValidator<boolean>) {
+  private getValidatorError(value: string, { message }: InputValidator) {
-    if (isFunction(message)) return message(value, this.props);
+    return typeof message === "function"
-
+      ? message(value, this.props)
-    return message || "";
+      : message;
   }
 
   private setupValidators() {

src/renderer/components/input/input_validators.ts

@@ -4,41 +4,113 @@
  */
 
 import type { InputProps } from "./input";
-import type { ReactNode } from "react";
+import type React from "react";
 import fse from "fs-extra";
 import { TypedRegEx } from "typed-regex";
+import type { SetRequired } from "type-fest";
 
-export class AsyncInputValidationError extends Error {
+export type InputValidationResult<IsAsync extends boolean> =
-}
+  IsAsync extends true
+    ? Promise<void>
+    : boolean;
 
-export type InputValidator<IsAsync extends boolean> = {
+export type InputValidation<IsAsync extends boolean> = (value: string, props?: InputProps) => InputValidationResult<IsAsync>;
+
+export type SyncValidationMessage = React.ReactNode | ((value: string, props?: InputProps) => React.ReactNode);
+
+export type InputValidator<IsAsync extends boolean = boolean> = {
   /**
    * Filters itself based on the input props
    */
   condition?: (props: InputProps) => any;
 } & (
-  IsAsync extends false
+  IsAsync extends true
     ? {
-      validate: (value: string, props: InputProps) => boolean;
+      /**
-      message: ReactNode | ((value: string, props: InputProps) => ReactNode | string);
+       * The validation message maybe either specified from the `message` field (higher priority)
-      debounce?: undefined;
+       * or if that is not provided then the message will retrived from the rejected with value
+       */
+      validate: InputValidation<true>;
+      message?: SyncValidationMessage;
+      debounce: number;
     }
     : {
-      /**
+      validate: InputValidation<false>;
-       * If asyncronous then the rejection message is the error message
+      message: SyncValidationMessage;
-       *
+      debounce?: undefined;
-       * This function MUST reject with an instance of {@link AsyncInputValidationError}
-       */
-      validate: (value: string, props: InputProps) => Promise<void>;
-      message?: undefined;
-      debounce: number;
     }
   );
 
-export function inputValidator<IsAsync extends boolean = false>(validator: InputValidator<IsAsync>): InputValidator<IsAsync> {
+export function isAsyncValidator(validator: InputValidator<boolean>): validator is InputValidator<true> {
+  return typeof validator.debounce === "number";
+}
+
+export function asyncInputValidator(validator: InputValidator<true>): InputValidator<true> {
   return validator;
 }
 
+export function inputValidator(validator: InputValidator<false>): InputValidator<false> {
+  return validator;
+}
+
+/**
+ * Create a new input validator from a list of syncronous input validators. Will match as valid if
+ * one of the input validators matches the input
+ */
+export function unionInputValidators(
+  baseValidator: Pick<InputValidator<false>, "condition" | "message">,
+  ...validators: InputValidator<false>[]
+): InputValidator<false> {
+  return inputValidator({
+    ...baseValidator,
+    validate: (value, props) => validators.some(validator => validator.validate(value, props)),
+  });
+}
+
+/**
+ * Create a new input validator from a list of syncronous or async input validators. Will match as
+ * valid if one of the input validators matches the input
+ */
+export function unionInputValidatorsAsync(
+  baseValidator: SetRequired<Pick<InputValidator<boolean>, "condition" | "message">, "message">,
+  ...validators: InputValidator<boolean>[]
+): InputValidator<true> {
+  const longestDebounce = Math.max(
+    ...validators
+      .filter(isAsyncValidator)
+      .map(validator => validator.debounce),
+    0,
+  );
+
+  return asyncInputValidator({
+    debounce: longestDebounce,
+    validate: async (value, props) => {
+      for (const validator of validators) {
+        if (isAsyncValidator(validator)) {
+          try {
+            await validator.validate(value, props);
+
+            return;
+          } catch {
+            // Do nothing
+          }
+        } else {
+          if (validator.validate(value, props)) {
+            return;
+          }
+        }
+      }
+
+      /**
+       * If no validator returns `true` then mark as invalid by throwing. The message will be
+       * obtained from the `message` field.
+       */
+      throw new Error();
+    },
+    ...baseValidator,
+  });
+}
+
 export const isRequired = inputValidator({
   condition: ({ required }) => required,
   message: () => `This field is required`,
@@ -53,7 +125,7 @@ export const isEmail = inputValidator({
 
 export const isNumber = inputValidator({
   condition: ({ type }) => type === "number",
-  message(value, { min, max }) {
+  message(value, { min, max } = {}) {
     const minMax: string = [
       typeof min === "number" ? `min: ${min}` : undefined,
       typeof max === "number" ? `max: ${max}` : undefined,
@@ -61,7 +133,7 @@ export const isNumber = inputValidator({
 
     return `Invalid number${minMax ? ` (${minMax})` : ""}`;
   },
-  validate: (value, { min, max }) => {
+  validate: (value, { min, max } = {}) => {
     const numVal = +value;
 
     return !(
@@ -100,30 +172,26 @@ export const isExtensionNameInstall = inputValidator({
   validate: value => isExtensionNameInstallRegex.isMatch(value),
 });
 
-export const isPath = inputValidator<true>({
+export const isPath = asyncInputValidator({
   debounce: 100,
   condition: ({ type }) => type === "text",
   validate: async value => {
-    try {
+    if (!await fse.pathExists(value)) {
-      await fse.pathExists(value);
+      throw new Error(`"${value}" is not a valid file path`);
-    } catch {
-      throw new AsyncInputValidationError(`${value} is not a valid file path`);
     }
   },
 });
 
 export const minLength = inputValidator({
   condition: ({ minLength }) => !!minLength,
-  message: (value, { minLength }) => `Minimum length is ${minLength}`,
+  message: (value, { minLength = 0 } = {}) => `Minimum length is ${minLength}`,
-  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+  validate: (value, { minLength = 0 } = {}) => value.length >= minLength,
-  validate: (value, { minLength }) => value.length >= minLength!,
 });
 
 export const maxLength = inputValidator({
   condition: ({ maxLength }) => !!maxLength,
-  message: (value, { maxLength }) => `Maximum length is ${maxLength}`,
+  message: (value, { maxLength = 0 } = {}) => `Maximum length is ${maxLength}`,
-  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+  validate: (value, { maxLength = 0 } = {}) => value.length <= maxLength,
-  validate: (value, { maxLength }) => value.length <= maxLength!,
 });
 
 const systemNameMatcher = /^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$/;
@@ -135,7 +203,7 @@ export const systemName = inputValidator({
 
 export const accountId = inputValidator({
   message: () => `Invalid account ID`,
-  validate: (value, props) => (isEmail.validate(value, props) || systemName.validate(value, props)),
+  validate: (value) => (isEmail.validate(value) || systemName.validate(value)),
 });
 
 export const conditionalValidators = [