diff --git a/app/scripts/modules/core/src/presentation/forms/FormField.tsx b/app/scripts/modules/core/src/presentation/forms/FormField.tsx index bc423f57f76..82abda43d5b 100644 --- a/app/scripts/modules/core/src/presentation/forms/FormField.tsx +++ b/app/scripts/modules/core/src/presentation/forms/FormField.tsx @@ -1,15 +1,12 @@ import * as React from 'react'; import { isNil } from 'lodash'; -import { IPromise } from 'angular'; -import { $q } from 'ngimport'; import { noop } from 'core/utils'; import { LayoutContext } from './layouts'; -import { useLatestPromise } from '../hooks'; import { createFieldValidator } from './FormikFormField'; import { renderContent } from './fields/renderContent'; -import { IValidator, IValidatorResultRaw } from './validation'; +import { IValidator } from './validation'; import { ICommonFormFieldProps, IControlledInputProps, @@ -49,17 +46,15 @@ export function FormField(props: IFormFieldProps) { const addValidator = useCallback((v: IValidator) => setInternalValidators(list => list.concat(v)), []); const removeValidator = useCallback((v: IValidator) => setInternalValidators(list => list.filter(x => x !== v)), []); + // Capture props.validate when the component initializes (to allow for inline arrow functions) const validate = useMemo(() => props.validate, []); + const fieldValidator = useMemo( () => createFieldValidator(label, required, [].concat(validate || noop).concat(internalValidators)), [label, required, validate], ); - const { result: errorMessage } = useLatestPromise( - // TODO: remove the following cast when we remove async validation from our API - () => $q.resolve((fieldValidator(value) as any) as IPromise), - [fieldValidator, value], - ); + const errorMessage = useMemo(() => fieldValidator(value), [fieldValidator, value]); const validationMessage = firstDefined(messageProp, errorMessage ? errorMessage : undefined); const validationStatus = firstDefined(statusProp, errorMessage ? 'error' : undefined); diff --git a/app/scripts/modules/core/src/presentation/forms/validation/validation.spec.ts b/app/scripts/modules/core/src/presentation/forms/validation/validation.spec.ts index cf433c259db..cce7e82e59f 100644 --- a/app/scripts/modules/core/src/presentation/forms/validation/validation.spec.ts +++ b/app/scripts/modules/core/src/presentation/forms/validation/validation.spec.ts @@ -1,18 +1,7 @@ import { Validators } from 'core/presentation/forms/validation/validators'; -import { buildValidators, IValidator, IArrayItemValidator, buildValidatorsAsync } from './validation'; +import { buildValidators, IValidator, IArrayItemValidator } from './validation'; -const { minValue, maxValue, arrayNotEmpty } = Validators; - -const makeAsync = (syncValidator: IValidator): IValidator => { - return (value, label) => { - const result = syncValidator(value, label); - return new Promise((resolve, reject) => { - setTimeout(() => { - result ? reject(result) : resolve(); - }, 1); - }); - }; -}; +const { maxValue, arrayNotEmpty } = Validators; describe('Synchronous validation', () => { it('returns empty errors when validating no validators for an optional field', () => { @@ -218,375 +207,3 @@ describe('Synchronous validation', () => { expect(result).toEqual(expectedResult); }); }); - -describe('Asynchronous validation of synchronous validators', () => { - it('returns empty errors when validating no validators - should also work with async', done => { - const values = { foo: 'bar' }; - - const builder = buildValidatorsAsync(values); - builder.field('foo', 'Foo').optional([]); - - const result = builder.result(); - const expectedResult = {}; - result.then((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); - - it('returns correct error when validating top level field - should also work with async', done => { - const values = {}; - - const builder = buildValidatorsAsync(values); - builder.field('foo', 'Foo').required(); - - const result = builder.result(); - const expectedResult = { - foo: 'Foo is required.', - }; - result.catch((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); - - it('returns correct error when validating a deep field - should also work with async', done => { - const values = {}; - - const builder = buildValidatorsAsync(values); - builder.field('foo.bar.baz', 'Foo').required(); - - const result = builder.result(); - const expectedResult = { - foo: { - bar: { - baz: 'Foo is required.', - }, - }, - }; - result.catch((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); - - it('aggregates multiple levels of errors correctly - should also work with async', done => { - const values = {}; - - const builder = buildValidatorsAsync(values); - builder.field('foo', 'Foo').required(); - builder.field('bar.baz', 'Baz').required(); - - const result = builder.result(); - const expectedResult = { - foo: 'Foo is required.', - bar: { - baz: 'Baz is required.', - }, - }; - result.catch((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); - - it('validates arrays and aggregates them correctly - should also work with async', done => { - const values = { - lotsastuff: [1, 2, 3, 4, 5], - }; - - const builder = buildValidatorsAsync(values); - const { arrayForEach } = builder; - - builder.field('lotsastuff', 'Array').required([ - arrayNotEmpty(), - arrayForEach(itemBuilder => { - itemBuilder.item('Item').required([maxValue(3)]); - }), - ]); - - const result = builder.result(); - const expectedResult = { - lotsastuff: [undefined, undefined, undefined, 'Item cannot be greater than 3', 'Item cannot be greater than 3'], - }; - result.catch((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); - - it('validates keys on array items and aggregates errors into resulting arrays correctly - should also work with async', done => { - const values = { - lotsastuff: [{ key: 1 }, { value: 2 }, 3, 4, 5], - }; - - const builder = buildValidatorsAsync(values); - const { arrayForEach } = builder; - builder.field('lotsastuff', 'Array').required([ - (array, label) => array.length < 1 && `${label} must have at least 1 item.`, - arrayForEach(itemBuilder => { - itemBuilder.field(`key`, `Item Key`).required(); - itemBuilder.field(`value`, `Item Value`).required(); - }), - ]); - - const result = builder.result(); - const expectedResult = { - lotsastuff: [ - { value: 'Item Value is required.' }, - { key: 'Item Key is required.' }, - { key: 'Item Key is required.', value: 'Item Value is required.' }, - { key: 'Item Key is required.', value: 'Item Value is required.' }, - { key: 'Item Key is required.', value: 'Item Value is required.' }, - ], - }; - result.catch((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); - - it('validates crazy complicated arrays of objects with arrays of objects - should also work with async', done => { - const values = { - letsgetcrazy: [ - {}, - { - key: 'array', - data: [ - { all: 1, of: 2, the: 3, things: 4 }, - { all: '', of: 2, the: 3, things: 4 }, - { all: 1, of: '', the: 3, things: 4 }, - { all: 1, of: 2, the: '', things: 4 }, - { all: 1, of: 2, the: 3, things: '' }, - {}, - ], - }, - { - key: 'nothotdog', - data: { foo: 'bar' }, - }, - ], - }; - - const builder = buildValidatorsAsync(values); - const isArray: IValidator = (array, label) => !Array.isArray(array) && `${label} must be an array.`; - const allOfTheThingsValidator: IArrayItemValidator = itemBuilder => { - itemBuilder.field(`all`, 'All').required(); - itemBuilder.field(`of`, 'Of').required(); - itemBuilder.field(`the`, 'The').required(); - itemBuilder.field(`things`, 'Things').required(); - }; - - const outerArrayItemValidator: IArrayItemValidator = itemBuilder => { - itemBuilder.field('key', 'Item key').required(); - itemBuilder.field('data', 'Item data').required([isArray, arrayForEach(allOfTheThingsValidator)]); - }; - - const { arrayForEach } = builder; - builder.field('letsgetcrazy', 'Outer array').required([arrayForEach(outerArrayItemValidator)]); - - const result = builder.result(); - const expectedResult = { - letsgetcrazy: [ - { key: 'Item key is required.', data: 'Item data is required.' }, - { - data: [ - undefined, - { all: 'All is required.' }, - { of: 'Of is required.' }, - { the: 'The is required.' }, - { things: 'Things is required.' }, - { - all: 'All is required.', - of: 'Of is required.', - the: 'The is required.', - things: 'Things is required.', - }, - ], - }, - { data: 'Item data must be an array.' }, - ], - }; - result.catch((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); -}); - -describe('Guarantees identical interfaces and results between sync and async', () => { - it('crazy complicated arrays of objects with arrays of objects', done => { - const isArray: IValidator = (array, label) => !Array.isArray(array) && `${label} must be an array.`; - const allOfTheThingsValidator: IArrayItemValidator = itemBuilder => { - itemBuilder.field(`all`, 'All').required(); - itemBuilder.field(`of`, 'Of').required(); - itemBuilder.field(`the`, 'The').required(); - itemBuilder.field(`things`, 'Things').required(); - }; - - const values = { - letsgetcrazy: [ - {}, - { - key: 'array', - data: [ - { all: 1, of: 2, the: 3, things: 4 }, - { all: '', of: 2, the: 3, things: 4 }, - { all: 1, of: '', the: 3, things: 4 }, - { all: 1, of: 2, the: '', things: 4 }, - { all: 1, of: 2, the: 3, things: '' }, - {}, - ], - }, - { - key: 'nothotdog', - data: { foo: 'bar' }, - }, - ], - }; - - const builderSync = buildValidators(values); - const builderAsync = buildValidatorsAsync(values); - - [builderSync, builderAsync].forEach(builder => { - const { arrayForEach } = builder; - const outerArrayItemValidator: IArrayItemValidator = itemBuilder => { - itemBuilder.field('key', 'Item key').required(); - itemBuilder.field('data', 'Item data').required([isArray, arrayForEach(allOfTheThingsValidator)]); - }; - builder.field('letsgetcrazy', 'Outer array').required([arrayForEach(outerArrayItemValidator)]); - }); - - const expectedResult = { - letsgetcrazy: [ - { key: 'Item key is required.', data: 'Item data is required.' }, - { - data: [ - undefined, - { all: 'All is required.' }, - { of: 'Of is required.' }, - { the: 'The is required.' }, - { things: 'Things is required.' }, - { - all: 'All is required.', - of: 'Of is required.', - the: 'The is required.', - things: 'Things is required.', - }, - ], - }, - { data: 'Item data must be an array.' }, - ], - }; - - const resultSync = builderSync.result(); - const resultAsync = builderAsync.result(); - - expect(resultSync).toEqual(expectedResult); - resultAsync.catch((errors: any) => { - expect(errors).toEqual(expectedResult); - done(); - }); - }); -}); - -describe('Asynchronous simple validation', () => { - it('Validate nothing', done => { - const values = { foo: 'bar' }; - - const builder = buildValidatorsAsync(values); - builder.field('foo', 'Foo').optional([]); - - builder.result().then((result: any) => { - expect(result).toEqual({}); - done(); - }); - }); - - it('Validate mixed sync/async', done => { - const values = { bar: 1, baz: 2 }; - - const builder = buildValidatorsAsync(values); - builder.field('foo', 'Foo').required(); - builder.field('bar', 'Bar').required([makeAsync(maxValue(2)), minValue(2)]); - builder.field('baz', 'Baz').required([minValue(1), makeAsync(maxValue(1))]); - - builder.result().catch((result: any) => { - expect(result).toEqual({ - foo: 'Foo is required.', - bar: 'Bar cannot be less than 2', - baz: 'Baz cannot be greater than 1', - }); - done(); - }); - }); -}); - -describe('Asynchronous array validation', () => { - it('Simple array validation', done => { - const values = { - lotsastuff: [1, 2, 3, 4, 5], - }; - const builder = buildValidators(values, true); - const { arrayForEach } = builder; - - builder.field('lotsastuff', 'Array').required([ - arrayForEach(itemBuilder => { - itemBuilder.item('Item').required([makeAsync(maxValue(3))]); - }), - ]); - - builder.result().catch((result: any) => { - expect(result).toEqual({ - lotsastuff: [undefined, undefined, undefined, 'Item cannot be greater than 3', 'Item cannot be greater than 3'], - }); - done(); - }); - }); -}); - -describe('Errors', () => { - it('Sneaking a promise into synchronous validation', () => { - const values = { foo: 'bar', bar: 'steve' }; - - const builder = buildValidators(values); - - expect(() => { - builder.field('foo', 'Foo').required([() => Promise.resolve(undefined)]); - }).toThrowError( - Error, - 'Synchronous validator cannot return a Promise (while validating foo). Use buildValidatorsAsync(values) instead.', - ); - expect(() => { - builder.field('bar', 'Bar').optional([() => Promise.reject('Bars should be awesome.')]); - }).toThrowError( - Error, - 'Synchronous validator cannot return a Promise (while validating bar). Use buildValidatorsAsync(values) instead.', - ); - }); - - it('Made one too many promises', done => { - const values = { - lotsastuff: [1, 2, 3, 4, 5], - }; - const builder = buildValidators(values, true); - const { arrayForEach } = builder; - - builder.field('lotsastuff', 'Array').required([ - makeAsync( - arrayForEach(itemBuilder => { - itemBuilder.item('Item').required([makeAsync(maxValue(3))]); - }), - ), - ]); - - builder.result().catch((error: any) => { - expect(error).toEqual( - new Error( - 'Warning: caught nested Promise while validating lotsastuff. Async Validators should only be rejecting undefined or string, not Promises.', - ), - ); - done(); - }); - }); -}); diff --git a/app/scripts/modules/core/src/presentation/forms/validation/validation.ts b/app/scripts/modules/core/src/presentation/forms/validation/validation.ts index 443906f64d8..38f825aa567 100644 --- a/app/scripts/modules/core/src/presentation/forms/validation/validation.ts +++ b/app/scripts/modules/core/src/presentation/forms/validation/validation.ts @@ -1,8 +1,7 @@ import { get, set } from 'lodash'; // Use Formik's Field.validate() api https://jaredpalmer.com/formik/docs/api/field#validate -export type IValidatorResultRaw = undefined | string; -export type IValidatorResult = IValidatorResultRaw | Promise; +export type IValidatorResult = undefined | string; export type IValidator = (value: any, label?: string) => IValidatorResult; export type IArrayItemValidator = ( itemBuilder: IArrayItemValidationBuilder, @@ -14,7 +13,7 @@ export type IArrayItemValidator = ( export interface IValidationBuilder { field: (name: string, label: string) => IValidatableField; - result: () => any | Promise; + result: () => any; arrayForEach: (iteratee: IArrayItemValidator) => IValidator; } @@ -24,36 +23,19 @@ interface INamedValidatorResult { } interface IValidatableField { - required: (validators?: IValidator[]) => undefined | Promise; - optional: (validators?: IValidator[]) => undefined | Promise; + required: (validators?: IValidator[]) => undefined; + optional: (validators?: IValidator[]) => undefined; } interface IArrayItemValidationBuilder extends IValidationBuilder { item: (label: string) => IValidatableField; } -const throwIfPromise = (maybe: any, message: string) => { - if (maybe && typeof maybe.then === 'function') { - throw new Error(message); - } - return maybe; -}; - -const validateSync = (validators: IValidator[], value: any, label: string, name: string) => { - const error = validators.reduce( +const runValidators = (validators: IValidator[], value: any, label: string) => { + return validators.reduce( (result, next) => (result ? result : next(value, label)), '', // Need a falsy ValidatorResult other than undefined, which will trip out Array.reduce() ); - return throwIfPromise( - error, - `Synchronous validator cannot return a Promise (while validating ${name}). Use buildValidatorsAsync(values) instead.`, - ); -}; - -const chainAsyncValidators = (validators: IValidator[], value: any, label: string) => { - return validators.reduce((p, next) => { - return p.then((result: IValidatorResult) => (result ? result : next(value, label))); - }, Promise.resolve(undefined)); }; const expandErrors = (errors: INamedValidatorResult[], isArray: boolean) => { @@ -103,9 +85,9 @@ const arrayForEach = (builder: (values: any) => IValidationBuilder, iteratee: IA }; }; -const buildValidatorsSync = (values: any): IValidationBuilder => { +export const buildValidators = (values: any): IValidationBuilder => { const isArray = Array.isArray(values); - const synchronousErrors: INamedValidatorResult[] = []; + const errors: INamedValidatorResult[] = []; return { field(name: string, label: string): IValidatableField { const value = get(values, name); @@ -113,7 +95,7 @@ const buildValidatorsSync = (values: any): IValidationBuilder => { required(validators = [], message?: string): undefined { if (value === undefined || value === null || value === '') { message = message || `${label} is required.`; - synchronousErrors.push({ name, error: message }); + errors.push({ name, error: message }); return undefined; } return this.optional(validators); @@ -123,77 +105,21 @@ const buildValidatorsSync = (values: any): IValidationBuilder => { // Don't run validation on an undefined/null/empty return undefined; } - const error = validateSync(validators, value, label, name); - synchronousErrors.push(isError(error) && { name, error }); + const error = runValidators(validators, value, label); + errors.push(isError(error) && { name, error }); return undefined; }, }; }, result(): any { - return expandErrors(synchronousErrors.filter(x => !!x), isArray); + return expandErrors(errors.filter(x => !!x), isArray); }, arrayForEach(iteratee: IArrayItemValidator) { - return arrayForEach(buildValidatorsSync, iteratee); + return arrayForEach(buildValidators, iteratee); }, }; }; -export const buildValidatorsAsync = (values: any): IValidationBuilder => { - const isArray = Array.isArray(values); - const promises: Array> = []; - return { - field(name, label) { - const value = get(values, name); - return { - required(validators = [], message?: string): Promise { - if (value === undefined || value === null || value === '') { - message = message || `${label} is required.`; - const chain = Promise.resolve({ name, error: message }); - promises.push(chain); - return chain; - } - return this.optional(validators); - }, - optional(validators: IValidator[]): Promise { - const chain: Promise = chainAsyncValidators(validators, value, label) - // We need to catch and resolve internal rejections because we'll be aggregating them using Promise.all() which fails fast - // and only rejects the first rejection. - .catch(error => - throwIfPromise( - error, - `Warning: caught nested Promise while validating ${name}. Async Validators should only be rejecting undefined or string, not Promises.`, - ), - ) - .then(error => isError(error) && { name, error }); - promises.push(chain); - return chain; - }, - }; - }, - result(): Promise { - return ( - Promise.all(promises) - // we don't need to catch() here because internal promises are individually caught inside validate() - .then(maybeErrors => { - const actuallyErrors = maybeErrors.filter(x => !!x); - if (actuallyErrors.length) { - return Promise.reject(expandErrors(actuallyErrors, isArray)); - } else { - return Promise.resolve({}); - } - }) - ); - }, - arrayForEach(iteratee: IArrayItemValidator) { - return arrayForEach(buildValidatorsAsync, iteratee); - }, - }; -}; - -export const buildValidators = (values: any, async?: boolean): IValidationBuilder => { - return async ? buildValidatorsAsync(values) : buildValidatorsSync(values); -}; - export const composeValidators = (validators: IValidator[]): IValidator => { const validatorList = validators.filter(x => !!x); if (!validatorList.length) { @@ -203,13 +129,9 @@ export const composeValidators = (validators: IValidator[]): IValidator => { } const composedValidators: IValidator = (value: any, label?: string) => { - const results: IValidatorResult[] = validatorList.map(validator => Promise.resolve(validator(value, label))); - + const results: IValidatorResult[] = validatorList.map(validator => validator(value, label)); // Return the first error returned from a validator - // Or return the first rejected promise (thrown/rejected by an async validator) - return Promise.all(results) - .then((errors: string[]) => errors.find(error => !!error)) - .catch((error: string) => error); + return results.find(error => !!error); }; return composedValidators;