Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: MNP of debounced validation #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 1 deletion packages/antd/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",
"test": "dts test",
"test:update": "dts test --u",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
2 changes: 1 addition & 1 deletion packages/bootstrap-4/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

Choose a reason for hiding this comment

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

I'm curious about the removal of this?

Copy link
Owner Author

Choose a reason for hiding this comment

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

As mentioned in the issue thread, this is not a real PR, just asking for validation on the generation direction:

It's in no way polished, and in its current state breaks some other functionality, but I wanted to consult it with you to assess whether it's worth continuing on this route. Would you please look at it? It this roughly acceptable as a concept, or did you imagine something else?

This was just a hack to get Husky to let me push. Maybe there's a easier way to do this, I've never worked with it before, but this seemed like an obvious way to shut him up :D


"test": "dts test",
"test:update": "dts test --u",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
2 changes: 1 addition & 1 deletion packages/chakra-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"test": "dts test",
"test:update": "dts test --u",
"test:watch": "dts test --watch",
Expand Down
1 change: 0 additions & 1 deletion packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
"cs-check": "prettier -l \"{src,test,testSnap}/**/*.[jt]s?(x)\"",
"cs-format": "prettier \"{src,test,testSnap}/**/*.[jt]s?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",
"publish-to-npm": "npm run build && npm publish",
"test": "dts test",
"test:debug": "node --inspect-brk node_modules/.bin/dts test",
Expand Down
145 changes: 89 additions & 56 deletions packages/core/src/components/Form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,14 @@ import _get from 'lodash/get';
import _isEmpty from 'lodash/isEmpty';
import _pick from 'lodash/pick';
import _toPath from 'lodash/toPath';
import _debounce from 'lodash/debounce';
import _memoize from 'lodash/memoize';

import getDefaultRegistry from '../getDefaultRegistry';

/** The properties that are passed to the `Form` */
export interface FormProps<T = any, S extends StrictRJSFSchema = RJSFSchema, F extends FormContextType = any> {
enableLogging?: boolean;
/** The JSON schema object for the form */
schema: S;
/** An implementation of the `ValidatorType` interface that is needed for form validation to work */
Expand Down Expand Up @@ -156,7 +159,7 @@ export interface FormProps<T = any, S extends StrictRJSFSchema = RJSFSchema, F e
/** If set to true, the form will perform validation and show any validation errors whenever the form data is changed,
* rather than just on submit
*/
liveValidate?: boolean;
liveValidate?: boolean | { debounceThreshold: number };
/** If `omitExtraData` and `liveOmit` are both set to true, then extra form data values that are not in any form field
* will be removed whenever `onChange` is called. Set to `false` by default
*/
Expand Down Expand Up @@ -274,9 +277,17 @@ export default class Form<
if (this.props.onChange && !deepEquals(this.state.formData, this.props.formData)) {
this.props.onChange(this.state);
}

this.validateAndUpdateState(this.state.formData);
this.formElement = createRef();
}

log = (...params: any[]) => {
if (this.props.enableLogging) {
console.log(...params);
}
};

/** React lifecycle method that gets called before new props are provided, updates the state based on new props. It
* will also call the`onChange` handler if the `formData` is modified to add missing default values as part of the
* state construction.
Expand All @@ -285,14 +296,30 @@ export default class Form<
*/
UNSAFE_componentWillReceiveProps(nextProps: FormProps<T, S, F>) {
const nextState = this.getStateFromProps(nextProps, nextProps.formData);
if (
const mustValidate = !nextProps.noValidate && nextProps.liveValidate;
const shouldCallOnChange =
!deepEquals(nextState.formData, nextProps.formData) &&
!deepEquals(nextState.formData, this.state.formData) &&
nextProps.onChange
) {
nextProps.onChange(nextState);
nextProps.onChange;

if (shouldCallOnChange) {
nextProps.onChange!(nextState);
}

this.setState(nextState);

if (mustValidate) {
// @ts-ignore
const debounceThreshold = nextProps.liveValidate?.debounceThreshold;

const callbackAfterValidation = () => {
if (shouldCallOnChange) {
nextProps.onChange!(this.state);
}
};

this.validateAndUpdateStateDebounced(debounceThreshold)(nextState.formData, callbackAfterValidation);
}
}

/** Extracts the updated state from the given `props` and `inputFormData`. As part of this process, the
Expand All @@ -308,8 +335,6 @@ export default class Form<
const schema = 'schema' in props ? props.schema : this.props.schema;
const uiSchema: UiSchema<T, S, F> = ('uiSchema' in props ? props.uiSchema! : this.props.uiSchema!) || {};
const edit = typeof inputFormData !== 'undefined';
const liveValidate = 'liveValidate' in props ? props.liveValidate : this.props.liveValidate;
const mustValidate = edit && !props.noValidate && liveValidate;
const rootSchema = schema;
const experimental_defaultFormStateBehavior =
'experimental_defaultFormStateBehavior' in props
Expand All @@ -328,38 +353,25 @@ export default class Form<
const getCurrentErrors = (): ValidationData<T> => {
if (props.noValidate) {
return { errors: [], errorSchema: {} };
} else if (!props.liveValidate) {
return {
errors: state.schemaValidationErrors || [],
errorSchema: state.schemaValidationErrorSchema || {},
};
}

// return {
// errors: state.schemaValidationErrors || [],
// errorSchema: state.schemaValidationErrorSchema || {},
// };

return {
errors: state.errors || [],
errorSchema: state.errorSchema || {},
};
};

let errors: RJSFValidationError[];
let errorSchema: ErrorSchema<T> | undefined;
let schemaValidationErrors: RJSFValidationError[] = state.schemaValidationErrors;
let schemaValidationErrorSchema: ErrorSchema<T> = state.schemaValidationErrorSchema;
if (mustValidate) {
const schemaValidation = this.validate(formData, schema, schemaUtils);
errors = schemaValidation.errors;
errorSchema = schemaValidation.errorSchema;
schemaValidationErrors = errors;
schemaValidationErrorSchema = errorSchema;
} else {
const currentErrors = getCurrentErrors();
errors = currentErrors.errors;
errorSchema = currentErrors.errorSchema;
}
if (props.extraErrors) {
const merged = validationDataMerge({ errorSchema, errors }, props.extraErrors);
errorSchema = merged.errorSchema;
errors = merged.errors;
}
const currentErrors = getCurrentErrors();
const errors: RJSFValidationError[] = currentErrors.errors;
const errorSchema: ErrorSchema<T> | undefined = currentErrors.errorSchema;
const schemaValidationErrors: RJSFValidationError[] = state.schemaValidationErrors;
const schemaValidationErrorSchema: ErrorSchema<T> = state.schemaValidationErrorSchema;

const idSchema = schemaUtils.toIdSchema(
retrievedSchema,
uiSchema['ui:rootFieldId'],
Expand Down Expand Up @@ -508,49 +520,70 @@ export default class Form<

const mustValidate = !noValidate && liveValidate;
let state: Partial<FormState<T, S, F>> = { formData, schema };
let newFormData = formData;

if (omitExtraData === true && liveOmit === true) {
const retrievedSchema = schemaUtils.retrieveSchema(schema, formData);
const pathSchema = schemaUtils.toPathSchema(retrievedSchema, '', formData);

const fieldNames = this.getFieldNames(pathSchema, formData);

newFormData = this.getUsedFormData(formData, fieldNames);

Choose a reason for hiding this comment

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

Why not keep this change since the state update below on 541 will be the old form data before omitting extra data?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Do you mean that this is a mistake, and L541 should get newFormData? I think this may be the case.

state = {
formData: newFormData,
formData: this.getUsedFormData(formData, fieldNames),
};
}

if (mustValidate) {
const schemaValidation = this.validate(newFormData);
let errors = schemaValidation.errors;
let errorSchema = schemaValidation.errorSchema;
const schemaValidationErrors = errors;
const schemaValidationErrorSchema = errorSchema;
if (extraErrors) {
const merged = validationDataMerge(schemaValidation, extraErrors);
errorSchema = merged.errorSchema;
errors = merged.errors;
}
state = {
formData: newFormData,
errors,
errorSchema,
schemaValidationErrors,
schemaValidationErrorSchema,
};
} else if (!noValidate && newErrorSchema) {
if (!noValidate && newErrorSchema) {
const errorSchema = extraErrors
? (mergeObjects(newErrorSchema, extraErrors, 'preventDuplicates') as ErrorSchema<T>)
: newErrorSchema;

state = {
formData: newFormData,
formData,
errorSchema: errorSchema,
errors: toErrorList(errorSchema),
};
}
this.setState(state as FormState<T, S, F>, () => onChange && onChange({ ...this.state, ...state }, id));

this.setState(state as FormState<T, S, F>, () => {
onChange && onChange({ ...this.state, ...state }, id);

if (mustValidate) {
// @ts-ignore

Choose a reason for hiding this comment

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

What are Typescript error is being disabled?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No idea, this is just to get it to compile, I would resolve all TS issues in the final PR

const debounceThreshold = liveValidate?.debounceThreshold;
const callbackAfterValidation = () => this.props.onChange?.(this.state, id);
this.validateAndUpdateStateDebounced(debounceThreshold)(state.formData, callbackAfterValidation);
}
});
};

validateAndUpdateStateDebounced = _memoize((debounceThreshold: number | null | undefined) => {

Choose a reason for hiding this comment

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

Does it make more sense to use the react version of memoize? or possibly create an instance variable for the debounce function and only change it when the debounceThreshold changes/is removed? Then you create the function in the constructor and in the componentDidUpdate

Copy link
Owner Author

@michal-kurz michal-kurz Jun 16, 2023

Choose a reason for hiding this comment

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

I'm not sure what you mean by "the react version of memoize", but using a single-line cache memoize implementation (something like memoize-one) would probably lead to a easier-to-read code :)

Choose a reason for hiding this comment

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

I mean React.useMemo()

return typeof debounceThreshold === 'number'
? (_debounce(this.validateAndUpdateState, debounceThreshold) as any)
: this.validateAndUpdateState;
});

validateAndUpdateState = (formData: T | undefined, callback?: () => void) => {
this.log('RUNNING VALIDATE!!', this.props.liveValidate);
const schemaValidation = this.validate(formData);
let errors = schemaValidation.errors;
let errorSchema = schemaValidation.errorSchema;
const schemaValidationErrors = errors;
const schemaValidationErrorSchema = errorSchema;

if (this.props.extraErrors) {
const merged = validationDataMerge(schemaValidation, this.props.extraErrors);
errorSchema = merged.errorSchema;
errors = merged.errors;
}

const state = {
errors,
errorSchema,
schemaValidationErrors,
schemaValidationErrorSchema,
};

this.setState(state as FormState<T, S, F>, callback);
};

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/docs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"write-translations": "docusaurus write-translations",
"write-heading-ids": "docusaurus write-heading-ids",
"typecheck": "tsc",
"precommit": "lint-staged",

"cs-check": "prettier -l \"{src, docs}/**/*.(ts?(x)|md|css)\"",
"cs-format": "prettier \"{src, docs}/**/*.(ts?(x)|md|css)\" --write",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
2 changes: 1 addition & 1 deletion packages/fluent-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"test": "dts test",
"test:update": "dts test --u",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"test": "dts test",
"test:update": "dts test --u",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
2 changes: 1 addition & 1 deletion packages/mui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"test": "dts test",
"test:update": "dts test --u",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
3 changes: 2 additions & 1 deletion packages/playground/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@
"cs-format": "prettier \"src/**/*.ts?(x)\" --write",
"build": "rimraf build && cross-env NODE_ENV=production vite build",
"lint": "eslint src",
"precommit": "lint-staged",

"publish-to-gh-pages": "npm run build && gh-pages --dist build/",
"publish-to-npm": "npm run build && npm publish",
"start": "vite --force",
"restart": "cd ../.. && npm run build && cd packages/playground && npm run start",
"preview": "vite preview",
"bump-packages": "npm update --save --lockfile-version 2"
},
Expand Down
2 changes: 2 additions & 0 deletions packages/playground/src/components/Playground.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,11 @@ export default function Playground({ themes, validators }: PlaygroundProps) {
>
<FormComponent
{...liveSettings}
enableLogging
templates={templates}
extraErrors={extraErrors}
schema={schema}
liveValidate={{ debounceThreshold: 1000 }}
uiSchema={uiSchema}
formData={formData}
fields={{ geo: GeoPosition }}
Expand Down
2 changes: 1 addition & 1 deletion packages/semantic-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"test": "dts test",
"test:update": "dts test --u",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"test": "dts test",
"test:debug": "node --inspect-brk node_modules/.bin/dts test",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down
2 changes: 1 addition & 1 deletion packages/validator-ajv6/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"test": "dts test",
"bump-packages": "npm update --save --lockfile-version 2"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/validator-ajv8/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"cs-check": "prettier -l \"{src,test}/**/*.ts?(x)\"",
"cs-format": "prettier \"{src,test}/**/*.ts?(x)\" --write",
"lint": "eslint src test",
"precommit": "lint-staged",

"compileSchemas": "node test/harness/compileTestSchema.js",
"test": "npm run compileSchemas && dts test",
"bump-packages": "npm update --save --lockfile-version 2"
Expand Down