From 20d03f10a49344608fe12ea857b427c81a1ba26a Mon Sep 17 00:00:00 2001 From: zombiej Date: Thu, 12 Nov 2020 22:07:08 +0800 Subject: [PATCH 1/7] fix: RenderProps deps validate --- src/Field.tsx | 6 +- src/interface.ts | 2 +- src/useForm.ts | 11 +-- tests/common/{index.js => index.ts} | 2 +- tests/{validate.test.js => validate.test.tsx} | 67 ++++++++++++++++++- 5 files changed, 78 insertions(+), 10 deletions(-) rename tests/common/{index.js => index.ts} (96%) rename tests/{validate.test.js => validate.test.tsx} (89%) diff --git a/src/Field.tsx b/src/Field.tsx index a05c0888..e66ba365 100644 --- a/src/Field.tsx +++ b/src/Field.tsx @@ -26,9 +26,9 @@ import { getValue, } from './utils/valueUtil'; -export type ShouldUpdate = +export type ShouldUpdate = | boolean - | ((prevValues: Store, nextValues: Store, info: { source?: string }) => boolean); + | ((prevValues: Values, nextValues: Values, info: { source?: string }) => boolean); function requireUpdate( shouldUpdate: ShouldUpdate, @@ -63,7 +63,7 @@ export interface InternalFieldProps { name?: InternalNamePath; normalize?: (value: StoreValue, prevValue: StoreValue, allValues: Store) => StoreValue; rules?: Rule[]; - shouldUpdate?: ShouldUpdate; + shouldUpdate?: ShouldUpdate; trigger?: string; validateTrigger?: string | string[] | false; validateFirst?: boolean | 'parallel'; diff --git a/src/interface.ts b/src/interface.ts index 433ebe5e..97f51794 100644 --- a/src/interface.ts +++ b/src/interface.ts @@ -46,7 +46,7 @@ type Validator = ( rule: RuleObject, value: StoreValue, callback: (error?: string) => void, -) => Promise | void; +) => Promise | void; export type RuleRender = (form: FormInstance) => RuleObject; diff --git a/src/useForm.ts b/src/useForm.ts index 1f90086d..d6542edd 100644 --- a/src/useForm.ts +++ b/src/useForm.ts @@ -578,12 +578,15 @@ export class FormStore { }); // Notify dependencies children with parent update + // We need delay to trigger validate in case Field is under render props const childrenFields = this.getDependencyChildrenFields(namePath); - this.validateFields(childrenFields); + Promise.resolve().then(() => { + this.validateFields(childrenFields); - this.notifyObservers(prevStore, childrenFields, { - type: 'dependenciesUpdate', - relatedFields: [namePath, ...childrenFields], + this.notifyObservers(prevStore, childrenFields, { + type: 'dependenciesUpdate', + relatedFields: [namePath, ...childrenFields], + }); }); // trigger callback function diff --git a/tests/common/index.js b/tests/common/index.ts similarity index 96% rename from tests/common/index.js rename to tests/common/index.ts index 88af2608..4d9b3cdd 100644 --- a/tests/common/index.js +++ b/tests/common/index.ts @@ -25,7 +25,7 @@ export function matchError(wrapper, error) { } } -export function getField(wrapper, index = 0) { +export function getField(wrapper, index: string | number = 0) { if (typeof index === 'number') { return wrapper.find(Field).at(index); } diff --git a/tests/validate.test.js b/tests/validate.test.tsx similarity index 89% rename from tests/validate.test.js rename to tests/validate.test.tsx index 4de387f6..da9f173b 100644 --- a/tests/validate.test.js +++ b/tests/validate.test.tsx @@ -498,7 +498,7 @@ describe('Form.Validate', () => { [ { name: 'serialization', first: true, second: false, validateFirst: true }, - { name: 'parallel', first: true, second: true, validateFirst: 'parallel' }, + { name: 'parallel', first: true, second: true, validateFirst: 'parallel' as const }, ].forEach(({ name, first, second, validateFirst }) => { it(name, async () => { let ruleFirst = false; @@ -610,5 +610,70 @@ describe('Form.Validate', () => { wrapper.find('button').simulate('click'); expect(renderProps.mock.calls[0][1]).toEqual(expect.objectContaining({ validating: true })); }); + + it('renderProps should use latest rules', async () => { + let failedTriggerTimes = 0; + let passedTriggerTimes = 0; + + interface FormStore { + username: string; + password: string; + } + + const Demo = () => ( +
+ + shouldUpdate={(prev, cur) => prev.username !== cur.username}> + {(_, __, { getFieldValue }) => { + const value = getFieldValue('username'); + + return ( + { + failedTriggerTimes += 1; + throw new Error('Failed'); + }, + }, + ] + : [ + { + validator: async () => { + passedTriggerTimes += 1; + }, + }, + ] + } + /> + ); + }} + + + ); + + const wrapper = mount(); + + expect(failedTriggerTimes).toEqual(0); + expect(passedTriggerTimes).toEqual(0); + + // Failed of second input + await changeValue(getField(wrapper, 1), ''); + matchError(getField(wrapper, 2), true); + + expect(failedTriggerTimes).toEqual(1); + expect(passedTriggerTimes).toEqual(0); + + // Changed first to trigger update + await changeValue(getField(wrapper, 0), 'light'); + matchError(getField(wrapper, 2), false); + + expect(failedTriggerTimes).toEqual(1); + expect(passedTriggerTimes).toEqual(1); + }); }); /* eslint-enable no-template-curly-in-string */ From 1282b859658d0c55ee9fdc75dd66a90112852617 Mon Sep 17 00:00:00 2001 From: zombiej Date: Fri, 13 Nov 2020 10:17:44 +0800 Subject: [PATCH 2/7] test: more test case --- tests/validate.test.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/validate.test.tsx b/tests/validate.test.tsx index da9f173b..04abb781 100644 --- a/tests/validate.test.tsx +++ b/tests/validate.test.tsx @@ -627,6 +627,10 @@ describe('Form.Validate', () => { {(_, __, { getFieldValue }) => { const value = getFieldValue('username'); + if (value === 'removed') { + return null; + } + return ( { expect(failedTriggerTimes).toEqual(1); expect(passedTriggerTimes).toEqual(1); + + // Remove should not trigger validate + await changeValue(getField(wrapper, 0), 'removed'); + + expect(failedTriggerTimes).toEqual(1); + expect(passedTriggerTimes).toEqual(1); }); }); /* eslint-enable no-template-curly-in-string */ From 21f06f2aa487b3dc5e26cd1c297efe38bbc8e928 Mon Sep 17 00:00:00 2001 From: zombiej Date: Fri, 13 Nov 2020 10:54:06 +0800 Subject: [PATCH 3/7] refactor: Move validate to field inner check --- src/Field.tsx | 84 +++++++++++++++------------- src/useForm.ts | 12 ++-- tests/legacy/dynamic-binding.test.js | 2 - 3 files changed, 51 insertions(+), 47 deletions(-) diff --git a/src/Field.tsx b/src/Field.tsx index e66ba365..9eb032c8 100644 --- a/src/Field.tsx +++ b/src/Field.tsx @@ -309,50 +309,56 @@ class Field extends React.Component implements F } }; - public validateRules = (options?: ValidateOptions) => { - const { validateFirst = false, messageVariables } = this.props; - const { triggerName } = (options || {}) as ValidateOptions; - const namePath = this.getNamePath(); + public validateRules = (options?: ValidateOptions): Promise => + // Force change to async to avoid rule OOD under renderProps field + Promise.resolve().then(() => { + if (!this.mounted) { + return []; + } - let filteredRules = this.getRules(); - if (triggerName) { - filteredRules = filteredRules.filter((rule: RuleObject) => { - const { validateTrigger } = rule; - if (!validateTrigger) { - return true; - } - const triggerList = toArray(validateTrigger); - return triggerList.includes(triggerName); - }); - } + const { validateFirst = false, messageVariables } = this.props; + const { triggerName } = (options || {}) as ValidateOptions; + const namePath = this.getNamePath(); - const promise = validateRules( - namePath, - this.getValue(), - filteredRules, - options, - validateFirst, - messageVariables, - ); - this.dirty = true; - this.validatePromise = promise; - this.errors = []; + let filteredRules = this.getRules(); + if (triggerName) { + filteredRules = filteredRules.filter((rule: RuleObject) => { + const { validateTrigger } = rule; + if (!validateTrigger) { + return true; + } + const triggerList = toArray(validateTrigger); + return triggerList.includes(triggerName); + }); + } - // Force trigger re-render since we need sync renderProps with new meta - this.reRender(); + const promise = validateRules( + namePath, + this.getValue(), + filteredRules, + options, + validateFirst, + messageVariables, + ); + this.dirty = true; + this.validatePromise = promise; + this.errors = []; - promise - .catch(e => e) - .then((errors: string[] = []) => { - if (this.validatePromise === promise) { - this.validatePromise = null; - this.errors = errors; - this.reRender(); - } - }); + // Force trigger re-render since we need sync renderProps with new meta + this.reRender(); - return promise; - }; + promise + .catch(e => e) + .then((errors: string[] = []) => { + if (this.validatePromise === promise) { + this.validatePromise = null; + this.errors = errors; + this.reRender(); + } + }); + + return promise; + }); public isFieldValidating = () => !!this.validatePromise; diff --git a/src/useForm.ts b/src/useForm.ts index d6542edd..cca49fe2 100644 --- a/src/useForm.ts +++ b/src/useForm.ts @@ -580,14 +580,14 @@ export class FormStore { // Notify dependencies children with parent update // We need delay to trigger validate in case Field is under render props const childrenFields = this.getDependencyChildrenFields(namePath); - Promise.resolve().then(() => { - this.validateFields(childrenFields); + // Promise.resolve().then(() => { + this.validateFields(childrenFields); - this.notifyObservers(prevStore, childrenFields, { - type: 'dependenciesUpdate', - relatedFields: [namePath, ...childrenFields], - }); + this.notifyObservers(prevStore, childrenFields, { + type: 'dependenciesUpdate', + relatedFields: [namePath, ...childrenFields], }); + // }); // trigger callback function const { onValuesChange } = this.callbacks; diff --git a/tests/legacy/dynamic-binding.test.js b/tests/legacy/dynamic-binding.test.js index 48214c33..b1b74ae1 100644 --- a/tests/legacy/dynamic-binding.test.js +++ b/tests/legacy/dynamic-binding.test.js @@ -2,8 +2,6 @@ import React from 'react'; import { mount } from 'enzyme'; import Form, { Field } from '../../src'; import { Input } from '../common/InfoField'; -import { changeValue, getField } from '../common'; -import timeout from '../common/timeout'; describe('legacy.dynamic-binding', () => { const getInput = (wrapper, id) => wrapper.find(id).last(); From 69f4db161184ec0a41d5204a87283cf99d50be63 Mon Sep 17 00:00:00 2001 From: zombiej Date: Fri, 13 Nov 2020 11:00:40 +0800 Subject: [PATCH 4/7] refactor: Re-order validate inner sep --- src/Field.tsx | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/Field.tsx b/src/Field.tsx index 9eb032c8..a8872645 100644 --- a/src/Field.tsx +++ b/src/Field.tsx @@ -309,9 +309,9 @@ class Field extends React.Component implements F } }; - public validateRules = (options?: ValidateOptions): Promise => + public validateRules = (options?: ValidateOptions): Promise => { // Force change to async to avoid rule OOD under renderProps field - Promise.resolve().then(() => { + const rootPromise = Promise.resolve().then(() => { if (!this.mounted) { return []; } @@ -340,17 +340,11 @@ class Field extends React.Component implements F validateFirst, messageVariables, ); - this.dirty = true; - this.validatePromise = promise; - this.errors = []; - - // Force trigger re-render since we need sync renderProps with new meta - this.reRender(); promise .catch(e => e) .then((errors: string[] = []) => { - if (this.validatePromise === promise) { + if (this.validatePromise === rootPromise) { this.validatePromise = null; this.errors = errors; this.reRender(); @@ -360,6 +354,16 @@ class Field extends React.Component implements F return promise; }); + this.validatePromise = rootPromise; + this.dirty = true; + this.errors = []; + + // Force trigger re-render since we need sync renderProps with new meta + this.reRender(); + + return rootPromise; + }; + public isFieldValidating = () => !!this.validatePromise; public isFieldTouched = () => this.touched; From ab1daa406344faffa6094ae9851e0ff0304da828 Mon Sep 17 00:00:00 2001 From: zombiej Date: Fri, 13 Nov 2020 11:18:07 +0800 Subject: [PATCH 5/7] fix: Validate should snapshot status --- src/Field.tsx | 7 +++++-- src/useForm.ts | 6 +++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Field.tsx b/src/Field.tsx index a8872645..5dc84435 100644 --- a/src/Field.tsx +++ b/src/Field.tsx @@ -310,6 +310,10 @@ class Field extends React.Component implements F }; public validateRules = (options?: ValidateOptions): Promise => { + // We should fixed namePath & value to avoid developer change then by form function + const namePath = this.getNamePath(); + const currentValue = this.getValue(); + // Force change to async to avoid rule OOD under renderProps field const rootPromise = Promise.resolve().then(() => { if (!this.mounted) { @@ -318,7 +322,6 @@ class Field extends React.Component implements F const { validateFirst = false, messageVariables } = this.props; const { triggerName } = (options || {}) as ValidateOptions; - const namePath = this.getNamePath(); let filteredRules = this.getRules(); if (triggerName) { @@ -334,7 +337,7 @@ class Field extends React.Component implements F const promise = validateRules( namePath, - this.getValue(), + currentValue, filteredRules, options, validateFirst, diff --git a/src/useForm.ts b/src/useForm.ts index cca49fe2..d1f8ea51 100644 --- a/src/useForm.ts +++ b/src/useForm.ts @@ -580,14 +580,14 @@ export class FormStore { // Notify dependencies children with parent update // We need delay to trigger validate in case Field is under render props const childrenFields = this.getDependencyChildrenFields(namePath); - // Promise.resolve().then(() => { - this.validateFields(childrenFields); + if (childrenFields.length) { + this.validateFields(childrenFields); + } this.notifyObservers(prevStore, childrenFields, { type: 'dependenciesUpdate', relatedFields: [namePath, ...childrenFields], }); - // }); // trigger callback function const { onValuesChange } = this.callbacks; From 8d32b64d35dc0e3e429eca8ebaca39f4fea28c90 Mon Sep 17 00:00:00 2001 From: zombiej Date: Fri, 13 Nov 2020 11:28:06 +0800 Subject: [PATCH 6/7] test: fix test case --- tests/dependencies.test.js | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/tests/dependencies.test.js b/tests/dependencies.test.js index 89ce1459..f7c4eae8 100644 --- a/tests/dependencies.test.js +++ b/tests/dependencies.test.js @@ -167,7 +167,7 @@ describe('Form.Dependencies', () => { const spy = jest.fn(); const wrapper = mount(
- true}> + true}> {() => { spy(); return 'gogogo'; @@ -182,27 +182,20 @@ describe('Form.Dependencies', () => { , ); expect(spy).toHaveBeenCalledTimes(1); - await changeValue(getField(wrapper, 2), 'value2'); + await changeValue(getField(wrapper, 1), 'value1'); // sync start // valueUpdate -> rerender by shouldUpdate // depsUpdate -> rerender by deps // [ react rerender once -> 2 ] // sync end - // async start - // validateFinish -> rerender by shouldUpdate - // [ react rerender once -> 3 ] - // async end - expect(spy).toHaveBeenCalledTimes(3); - await changeValue(getField(wrapper, 1), 'value1'); + expect(spy).toHaveBeenCalledTimes(2); + + await changeValue(getField(wrapper, 2), 'value2'); // sync start // valueUpdate -> rerender by shouldUpdate // depsUpdate -> rerender by deps - // [ react rerender once -> 4 ] + // [ react rerender once -> 3 ] // sync end - // async start - // validateFinish -> rerender by shouldUpdate - // [ react rerender once -> 5 ] - // async end - expect(spy).toHaveBeenCalledTimes(5); + expect(spy).toHaveBeenCalledTimes(3); }); }); From 946ce44f44a26e26d7c78857cebbcbc8d623538a Mon Sep 17 00:00:00 2001 From: zombiej Date: Fri, 13 Nov 2020 11:40:04 +0800 Subject: [PATCH 7/7] test: fix test case --- tests/common/InfoField.tsx | 26 +++++++++++++++----------- tests/validate.test.tsx | 1 + 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/tests/common/InfoField.tsx b/tests/common/InfoField.tsx index 2acd6f52..54613067 100644 --- a/tests/common/InfoField.tsx +++ b/tests/common/InfoField.tsx @@ -13,17 +13,21 @@ export const Input = ({ value = '', ...props }) => = ({ children, ...props }) => ( - {(control, { errors, validating }) => ( -
- {children ? React.cloneElement(children, control) : } -
    - {errors.map((error, index) => ( -
  • {error}
    • - ))} -
- {validating && } -
- )} + {(control, info) => { + const { errors, validating } = info; + + return ( +
+ {children ? React.cloneElement(children, control) : } +
    + {errors.map((error, index) => ( +
  • {error}
    • + ))} +
+ {validating && } +
+ ); + }} ); diff --git a/tests/validate.test.tsx b/tests/validate.test.tsx index 04abb781..c1d2e865 100644 --- a/tests/validate.test.tsx +++ b/tests/validate.test.tsx @@ -532,6 +532,7 @@ describe('Form.Validate', () => { await changeValue(wrapper, 'test'); await timeout(); + wrapper.update(); matchError(wrapper, 'failed first'); expect(ruleFirst).toEqual(first);