WIP: MNP of debounced validation #1
Conversation
| @@ -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", | |||
There was a problem hiding this comment.
I'm curious about the removal of this?
There was a problem hiding this comment.
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
| onChange && onChange({ ...this.state, ...state }, id); | ||
|
|
||
| if (mustValidate) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
What are Typescript error is being disabled?
There was a problem hiding this comment.
No idea, this is just to get it to compile, I would resolve all TS issues in the final PR
|
|
||
| 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); |
There was a problem hiding this comment.
Why not keep this change since the state update below on 541 will be the old form data before omitting extra data?
There was a problem hiding this comment.
Do you mean that this is a mistake, and L541 should get newFormData? I think this may be the case.
| }); | ||
| }; | ||
|
|
||
| validateAndUpdateStateDebounced = _memoize((debounceThreshold: number | null | undefined) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
Reasons for making this change
[Please describe them here]
If this is related to existing tickets, include links to them as well. Use the syntax
fixes #[issue number](ex:fixes #123).If your PR is non-trivial and you'd like to schedule a synchronous review, please add it to the weekly meeting agenda: https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit
Checklist
npm run test:updateto update snapshots, if needed.