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>
2025-05-20 05:10:56 +00:00 · 2022-06-10 06:41:29 -04:00 · 2022-06-10 06:41:29 -04:00 · 5420780ae0
commit 5420780ae0
parent 3d64425df2
6 changed files with 244 additions and 98 deletions
--- a/src/common/utils/type-narrowing.ts
+++ b/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)
--- a/src/renderer/components/+extensions/install-from-input/install-from-input.tsx
+++ b/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) {
-            const { name, version } = extNameCaptures;
+        if (extNameCaptures) {
+          const { name, version } = extNameCaptures;

-            return await attemptInstallByInfo({ name, version });
-          }
-        } else {
-          throw error;
+          return await attemptInstallByInfo({ name, version });
        }
      }

--- a/src/renderer/components/+extensions/install.tsx
+++ b/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
-  from "../../../extensions/extension-installation-state-store/extension-installation-state-store.injectable";
+import extensionInstallationStateStoreInjectable 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,
-];
-
-const installInputValidator = inputValidator({
-  message: "Invalid URL, absolute path, or extension name",
-  validate: (value: string, props) => (
-    installInputValidators.some(({ validate }) => validate(value, props))
-  ),
-});
+  InputValidators.isPath,
+);

 const NonInjectedInstall: React.FC<Dependencies & InstallProps> = ({
  installPath,
--- a/src/renderer/components/input/tests/input_validators.test.ts
+++ b/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);
    });
  });
 });
--- a/src/renderer/components/input/input.tsx
+++ b/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 isFunction from "lodash/isFunction";
+import type { InputValidator, InputValidation, InputValidationResult, SyncValidationMessage } from "./input_validators";
 import uniqueId from "lodash/uniqueId";
 import { debounce } from "lodash";

-const { conditionalValidators, ...InputValidators } = Validators;
+const {
+  conditionalValidators,
+  asyncInputValidator,
+  inputValidator,
+  isAsyncValidator,
+  unionInputValidatorsAsync,
+  ...InputValidators
+} = Validators;

-export { InputValidators };
-export type { InputValidator };
+export {
+  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(
-          validator.validate(value, this.props).then(
-            () => null, // don't consider any valid result from promise since we interested in errors only
-            error => this.getValidatorError(value, validator) || error,
-          ),
-        );
-      }
-
-      const isValid = validator.validate(value, this.props);
-
-      if (isValid === false) {
+        asyncValidators.push((async () => {
+          try {
+            await validator.validate(value, this.props);
+          } catch (error) {
+            return this.getValidatorError(value, validator) || (error instanceof Error ? error.message : String(error));
+          }
+        })());
+      } else if (!validator.validate(value, this.props)) {
        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>) {
-    if (isFunction(message)) return message(value, this.props);
-
-    return message || "";
+  private getValidatorError(value: string, { message }: InputValidator) {
+    return typeof message === "function"
+      ? message(value, this.props)
+      : message;
  }

  private setupValidators() {
--- a/src/renderer/components/input/input_validators.ts
+++ b/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);
-      debounce?: undefined;
+      /**
+       * The validation message maybe either specified from the `message` field (higher priority)
+       * or if that is not provided then the message will retrived from the rejected with value
+       */
+      validate: InputValidation<true>;
+      message?: SyncValidationMessage;
+      debounce: number;
    }
    : {
-      /**
-       * If asyncronous then the rejection message is the error message
-       *
-       * This function MUST reject with an instance of {@link AsyncInputValidationError}
-       */
-      validate: (value: string, props: InputProps) => Promise<void>;
-      message?: undefined;
-      debounce: number;
+      validate: InputValidation<false>;
+      message: SyncValidationMessage;
+      debounce?: undefined;
    }
  );

-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 {
-      await fse.pathExists(value);
-    } catch {
-      throw new AsyncInputValidationError(`${value} is not a valid file path`);
+    if (!await fse.pathExists(value)) {
+      throw new Error(`"${value}" is not a valid file path`);
    }
  },
 });

 export const minLength = inputValidator({
  condition: ({ minLength }) => !!minLength,
-  message: (value, { minLength }) => `Minimum length is ${minLength}`,
-  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-  validate: (value, { minLength }) => value.length >= minLength!,
+  message: (value, { minLength = 0 } = {}) => `Minimum length is ${minLength}`,
+  validate: (value, { minLength = 0 } = {}) => value.length >= minLength,
 });

 export const maxLength = inputValidator({
  condition: ({ maxLength }) => !!maxLength,
-  message: (value, { maxLength }) => `Maximum length is ${maxLength}`,
-  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-  validate: (value, { maxLength }) => value.length <= maxLength!,
+  message: (value, { maxLength = 0 } = {}) => `Maximum length is ${maxLength}`,
+  validate: (value, { maxLength = 0 } = {}) => 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 = [