From 5420780ae0af5da14c0dbcd5bba073e7d38e26e5 Mon Sep 17 00:00:00 2001 From: Sebastian Malton Date: Fri, 10 Jun 2022 06:41:29 -0400 Subject: [PATCH] Fix changes to InputValidator to resolve accidental breaking changes (#5403) * Fix changes to InputValidator to resolve accidental breaking changes Signed-off-by: Sebastian Malton * Show changes working by removing no longer required empty props objects Signed-off-by: Sebastian Malton * fix type errors Signed-off-by: Sebastian Malton * Fix async validation errors not being displayed - And fix validation errors sometimes being displayed multiple times Signed-off-by: Sebastian Malton * 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 * Introduce unionizing functions for input validators Signed-off-by: Sebastian Malton * fix tests Signed-off-by: Sebastian Malton * Remove RequiredProps type param Signed-off-by: Sebastian Malton --- src/common/utils/type-narrowing.ts | 8 ++ .../install-from-input/install-from-input.tsx | 17 +-- .../components/+extensions/install.tsx | 21 ++- .../input/__tests__/input_validators.test.ts | 83 ++++++++++- src/renderer/components/input/input.tsx | 83 +++++------ .../components/input/input_validators.ts | 130 +++++++++++++----- 6 files changed, 244 insertions(+), 98 deletions(-) diff --git a/src/common/utils/type-narrowing.ts b/src/common/utils/type-narrowing.ts index a138b2a0ec..eb5b6996cf 100644 --- a/src/common/utils/type-narrowing.ts +++ b/src/common/utils/type-narrowing.ts @@ -137,6 +137,14 @@ export function hasDefiniteField(field: Field): (val: return (val): val is T & { [f in Field]-?: NonNullable } => val[field] != null; } +export function isPromiseSettledRejected(result: PromiseSettledResult): result is PromiseRejectedResult { + return result.status === "rejected"; +} + +export function isPromiseSettledFulfilled(result: PromiseSettledResult): result is PromiseFulfilledResult { + return result.status === "fulfilled"; +} + export function isErrnoException(error: unknown): error is NodeJS.ErrnoException { return isObject(error) && hasOptionalTypedProperty(error, "code", isString) diff --git a/src/renderer/components/+extensions/install-from-input/install-from-input.tsx b/src/renderer/components/+extensions/install-from-input/install-from-input.tsx index ab85221421..eb5d6febe4 100644 --- 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; @@ -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 }); } } diff --git a/src/renderer/components/+extensions/install.tsx b/src/renderer/components/+extensions/install.tsx index 52c0bd8906..7fa192e3a2 100644 --- 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 = ({ installPath, diff --git a/src/renderer/components/input/__tests__/input_validators.test.ts b/src/renderer/components/input/__tests__/input_validators.test.ts index ae45b998dd..cb6a0b3db9 100644 --- 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); }); }); }); diff --git a/src/renderer/components/input/input.tsx b/src/renderer/components/input/input.tsx index 9514e796e3..f45c8377a0 100644 --- 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 = { blurOnEnter: true, }; -function isAsyncValidator(validator: InputValidator): validator is InputValidator { - return typeof validator.debounce === "number"; -} - export class Input extends React.Component { static defaultProps = defaultProps as object; public input: InputElement | null = null; - public validators: InputValidator[] = []; + public validators: InputValidator[] = []; public state: State = { focused: false, @@ -155,7 +168,7 @@ export class Input extends React.Component { async validate() { const value = this.getValue(); let validationId = (this.validationId = ""); // reset every time for async validators - const asyncValidators: Promise[] = []; + const asyncValidators: Promise[] = []; const errors: React.ReactNode[] = []; // run validators @@ -169,28 +182,15 @@ export class Input extends React.Component { 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 { // 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 { }); } - private getValidatorError(value: string, { message }: InputValidator) { - 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() { diff --git a/src/renderer/components/input/input_validators.ts b/src/renderer/components/input/input_validators.ts index a9844bfcd8..59e12a977a 100644 --- 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 true + ? Promise + : boolean; -export type InputValidator = { +export type InputValidation = (value: string, props?: InputProps) => InputValidationResult; + +export type SyncValidationMessage = React.ReactNode | ((value: string, props?: InputProps) => React.ReactNode); + +export type InputValidator = { /** * 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; + 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; - message?: undefined; - debounce: number; + validate: InputValidation; + message: SyncValidationMessage; + debounce?: undefined; } ); -export function inputValidator(validator: InputValidator): InputValidator { +export function isAsyncValidator(validator: InputValidator): validator is InputValidator { + return typeof validator.debounce === "number"; +} + +export function asyncInputValidator(validator: InputValidator): InputValidator { return validator; } +export function inputValidator(validator: InputValidator): InputValidator { + 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, "condition" | "message">, + ...validators: InputValidator[] +): InputValidator { + 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, "condition" | "message">, "message">, + ...validators: InputValidator[] +): InputValidator { + 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({ +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 = [