Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Memoize field validation results #10714

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Memoize field validation results #10714

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2c8675
Memoize field validation results
t3chguy Apr 26, 2023
8bb2a51
Make validation memoization opt-in
t3chguy Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
9 changes: 8 additions & 1 deletion src/components/views/elements/Field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -195,12 +196,18 @@ export default class Field extends React.PureComponent<PropShapes, IState> {
this.props.onBlur?.(ev);
};

// Memoize latest result as some validation functions can be costly, e.g. API hits
private onValidate = memoizeOne(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use memoizeOne over memoize in lodash?
I understand they work slightly differently, but i'm wondering what is the rationale behind only memoizing the last validation call?

Is it to save memory? Or are there cases where the validation functions aren't pure?

My preference would be to get rid of memoize-one unless there's a compelling reason to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realise lodash had a memoize, good to know. I was liking memoize-one because

  1. react-beautiful-dnd already put it into our dep stack
  2. it behaves like useMemo/React.memo

Lodash memoize doesn't seem to have any facility to limit the cache size so it'll mean adding a bunch of boilerplate to get what we want out of it. We just want to prevent running the exact same call multiple times, if the user keeps removing & entering the same query we don't care

(fieldState: IFieldState): Promise<IValidationResult> => 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<boolean | undefined> {
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,
Expand Down