From c2c86753086e59627c29b451b98451438ef7b870 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 26 Apr 2023 15:19:21 +0100 Subject: [PATCH 1/2] Memoize field validation results --- package.json | 1 + src/components/views/elements/Field.tsx | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index d7b7997d878..7e110a52de4 100644 --- a/package.json +++ b/package.json @@ -98,6 +98,7 @@ "matrix-events-sdk": "0.0.1", "matrix-js-sdk": "github:matrix-org/matrix-js-sdk#develop", "matrix-widget-api": "^1.3.1", + "memoize-one": "^5.1.1", "minimist": "^1.2.5", "opus-recorder": "^8.0.3", "pako": "^2.0.3", diff --git a/src/components/views/elements/Field.tsx b/src/components/views/elements/Field.tsx index a453ffbee54..2eb63d0a384 100644 --- a/src/components/views/elements/Field.tsx +++ b/src/components/views/elements/Field.tsx @@ -17,6 +17,7 @@ limitations under the License. import React, { InputHTMLAttributes, SelectHTMLAttributes, TextareaHTMLAttributes, RefObject } from "react"; import classNames from "classnames"; import { debounce } from "lodash"; +import memoizeOne from "memoize-one"; import { IFieldState, IValidationResult } from "./Validation"; import Tooltip from "./Tooltip"; @@ -195,12 +196,18 @@ export default class Field extends React.PureComponent { this.props.onBlur?.(ev); }; + // Memoize latest result as some validation functions can be costly, e.g. API hits + private onValidate = memoizeOne( + (fieldState: IFieldState): Promise => this.props.onValidate!(fieldState), + ([a], [b]) => a.value === b.value && a.focused === b.focused && a.allowEmpty === b.allowEmpty, + ); + public async validate({ focused, allowEmpty = true }: IValidateOpts): Promise { if (!this.props.onValidate) { return; } const value = this.inputRef.current?.value ?? null; - const { valid, feedback } = await this.props.onValidate({ + const { valid, feedback } = await this.onValidate({ value, focused: !!focused, allowEmpty, From 8bb2a5194ed2c7ab7a3cbbf0812d26dba49edc11 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 27 Apr 2023 08:12:08 +0100 Subject: [PATCH 2/2] Make validation memoization opt-in --- src/components/views/auth/PassphraseField.tsx | 1 + .../views/directory/NetworkDropdown.tsx | 1 + src/components/views/elements/Field.tsx | 9 +- src/components/views/elements/Validation.tsx | 137 +++++++++++------- 4 files changed, 85 insertions(+), 63 deletions(-) diff --git a/src/components/views/auth/PassphraseField.tsx b/src/components/views/auth/PassphraseField.tsx index 1050e8f6e22..3f232923ccc 100644 --- a/src/components/views/auth/PassphraseField.tsx +++ b/src/components/views/auth/PassphraseField.tsx @@ -92,6 +92,7 @@ class PassphraseField extends PureComponent { }, }, ], + memoize: true, }); public onValidate = async (fieldState: IFieldState): Promise => { diff --git a/src/components/views/directory/NetworkDropdown.tsx b/src/components/views/directory/NetworkDropdown.tsx index b90d22574b6..b4ed2e10aea 100644 --- a/src/components/views/directory/NetworkDropdown.tsx +++ b/src/components/views/directory/NetworkDropdown.tsx @@ -68,6 +68,7 @@ const validServer = withValidation({ : _t("Can't find this server or its room list"), }, ], + memoize: true, }); function useSettingsValueWithSetter( diff --git a/src/components/views/elements/Field.tsx b/src/components/views/elements/Field.tsx index 2eb63d0a384..a453ffbee54 100644 --- a/src/components/views/elements/Field.tsx +++ b/src/components/views/elements/Field.tsx @@ -17,7 +17,6 @@ limitations under the License. import React, { InputHTMLAttributes, SelectHTMLAttributes, TextareaHTMLAttributes, RefObject } from "react"; import classNames from "classnames"; import { debounce } from "lodash"; -import memoizeOne from "memoize-one"; import { IFieldState, IValidationResult } from "./Validation"; import Tooltip from "./Tooltip"; @@ -196,18 +195,12 @@ export default class Field extends React.PureComponent { this.props.onBlur?.(ev); }; - // Memoize latest result as some validation functions can be costly, e.g. API hits - private onValidate = memoizeOne( - (fieldState: IFieldState): Promise => this.props.onValidate!(fieldState), - ([a], [b]) => a.value === b.value && a.focused === b.focused && a.allowEmpty === b.allowEmpty, - ); - public async validate({ focused, allowEmpty = true }: IValidateOpts): Promise { if (!this.props.onValidate) { return; } const value = this.inputRef.current?.value ?? null; - const { valid, feedback } = await this.onValidate({ + const { valid, feedback } = await this.props.onValidate({ value, focused: !!focused, allowEmpty, diff --git a/src/components/views/elements/Validation.tsx b/src/components/views/elements/Validation.tsx index b1101f41dc2..c0f21826f2f 100644 --- a/src/components/views/elements/Validation.tsx +++ b/src/components/views/elements/Validation.tsx @@ -15,8 +15,9 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { ReactNode } from "react"; +import React, { ReactChild, ReactNode } from "react"; import classNames from "classnames"; +import memoizeOne from "memoize-one"; type Data = Pick; @@ -40,6 +41,7 @@ interface IArgs { description?(this: T, derivedData: D, results: IResult[]): ReactNode; hideDescriptionIfValid?: boolean; deriveData?(data: Data): Promise; + memoize?: boolean; } export interface IFieldState { @@ -60,7 +62,7 @@ export interface IValidationResult { * @param {Function} description * Function that returns a string summary of the kind of value that will * meet the validation rules. Shown at the top of the validation feedback. - * @param {Boolean} hideDescriptionIfValid + * @param {boolean} hideDescriptionIfValid * If true, don't show the description if the validation passes validation. * @param {Function} deriveData * Optional function that returns a Promise to an object of generic type D. @@ -75,6 +77,9 @@ export interface IValidationResult { * - `valid`: Function returning text to show when the rule is valid. Only shown if set. * - `invalid`: Function returning text to show when the rule is invalid. Only shown if set. * - `final`: A Boolean if true states that this rule will only be considered if all rules before it returned valid. + * @param {boolean?} memoize + * If true, will use memoization to avoid calling deriveData & rules unless the value or allowEmpty change. + * Be careful to not use this if your validation is not pure and depends on other fields, such as "repeat password". * @returns {Function} * A validation function that takes in the current input value and returns * the overall validity and a feedback UI that can be rendered for more detail. @@ -84,73 +89,87 @@ export default function withValidation({ hideDescriptionIfValid, deriveData, rules, -}: IArgs) { - return async function onValidate( + memoize, +}: IArgs): (fieldState: IFieldState) => Promise { + let checkRules = async function ( this: T, - { value, focused, allowEmpty = true }: IFieldState, - ): Promise { - if (!value && allowEmpty) { - return {}; - } - - const data = { value, allowEmpty }; - // We know that if deriveData is set then D will not be undefined - const derivedData: D = (await deriveData?.call(this, data)) as D; - + data: Data, + derivedData: D, + ): Promise<[valid: boolean, results: IResult[]]> { const results: IResult[] = []; let valid = true; - if (rules?.length) { - for (const rule of rules) { - if (!rule.key || !rule.test) { - continue; - } + for (const rule of rules) { + if (!rule.key || !rule.test) { + continue; + } - if (!valid && rule.final) { - continue; - } + if (!valid && rule.final) { + continue; + } + + if (rule.skip?.call(this, data, derivedData)) { + continue; + } - if (rule.skip?.call(this, data, derivedData)) { + // We're setting `this` to whichever component holds the validation + // function. That allows rules to access the state of the component. + const ruleValid: boolean = await rule.test.call(this, data, derivedData); + valid = valid && ruleValid; + if (ruleValid && rule.valid) { + // If the rule's result is valid and has text to show for + // the valid state, show it. + const text = rule.valid.call(this, derivedData); + if (!text) { continue; } - - // We're setting `this` to whichever component holds the validation - // function. That allows rules to access the state of the component. - const ruleValid: boolean = await rule.test.call(this, data, derivedData); - valid = valid && ruleValid; - if (ruleValid && rule.valid) { - // If the rule's result is valid and has text to show for - // the valid state, show it. - const text = rule.valid.call(this, derivedData); - if (!text) { - continue; - } - results.push({ - key: rule.key, - valid: true, - text, - }); - } else if (!ruleValid && rule.invalid) { - // If the rule's result is invalid and has text to show for - // the invalid state, show it. - const text = rule.invalid.call(this, derivedData); - if (!text) { - continue; - } - results.push({ - key: rule.key, - valid: false, - text, - }); + results.push({ + key: rule.key, + valid: true, + text, + }); + } else if (!ruleValid && rule.invalid) { + // If the rule's result is invalid and has text to show for + // the invalid state, show it. + const text = rule.invalid.call(this, derivedData); + if (!text) { + continue; } + results.push({ + key: rule.key, + valid: false, + text, + }); } } + return [valid, results]; + }; + + // We have to memoize it in chunks as `focused` can change frequently, but it isn't passed to these methods + if (memoize) { + if (deriveData) deriveData = memoizeOne(deriveData, isDataEqual); + checkRules = memoizeOne(checkRules, isDerivedDataEqual); + } + + return async function onValidate( + this: T, + { value, focused, allowEmpty = true }: IFieldState, + ): Promise { + if (!value && allowEmpty) { + return {}; + } + + const data = { value, allowEmpty }; + // We know that if deriveData is set then D will not be undefined + const derivedData = (await deriveData?.call(this, data)) as D; + const [valid, results] = await checkRules.call(this, data, derivedData); + // Hide feedback when not focused if (!focused) { return { valid }; } - let details; + let details: ReactNode | undefined; if (results && results.length) { details = (
    @@ -170,7 +189,7 @@ export default function withValidation({ ); } - let summary; + let summary: ReactNode | undefined; if (description && (details || !hideDescriptionIfValid)) { // We're setting `this` to whichever component holds the validation // function. That allows rules to access the state of the component. @@ -178,7 +197,7 @@ export default function withValidation({ summary = content ?
    {content}
    : undefined; } - let feedback; + let feedback: ReactChild | undefined; if (summary || details) { feedback = (
    @@ -194,3 +213,11 @@ export default function withValidation({ }; }; } + +function isDataEqual([a]: [Data], [b]: [Data]): boolean { + return a.value === b.value && a.allowEmpty === b.allowEmpty; +} + +function isDerivedDataEqual([a1, a2]: [Data, any], [b1, b2]: [Data, any]): boolean { + return a2 === b2 && isDataEqual([a1], [b1]); +}