-
Notifications
You must be signed in to change notification settings - Fork 0
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
Debounce submitCustom #1205
Debounce submitCustom #1205
Conversation
db1b046
to
aa025d4
Compare
7a68536
to
6fa1baf
Compare
…thery-org/feathery-react into singh/debounce-submit-custom
src/utils/featheryClient/index.ts
Outdated
this.flushPendingSubmitCustomUpdates = (override: boolean = true) => { | ||
// we call the debounced method and then the flush to immediately submit changes | ||
// see: https://github.com/lodash/lodash/issues/4185#issuecomment-462388355 | ||
this.debouncedSubmitCustom(override); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added: to immediately submit changes we must call the flush method in this way
src/utils/featheryClient/index.ts
Outdated
// we call the debounced method and then the flush to immediately submit changes | ||
// see: https://github.com/lodash/lodash/issues/4185#issuecomment-462388355 | ||
this.debouncedSubmitCustom(override ?? true); | ||
return this.debouncedSubmitCustom.flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not familiar with the flush function. will this return the promise from the network request so it can be awaited by the client of this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/utils/featheryClient/index.ts
Outdated
this._debouncedSubmitCustom.bind(this), | ||
SUBMIT_CUSTOM_DEBOUNCE_WINDOW | ||
); | ||
this.flushPendingSubmitCustomUpdates = (override?: boolean) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure this method is necessary actually. if there is data to flush, there will be a debounced, pending submitCustom call as well so no need to call this again or have this helper function. .flush() can be called directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think wrapping the flush call in this method creates for a cleaner API long term.
There's currently 3 instances where we flush. Since the lodash.flush
implementation has this caveat that requires us to
- call the debounced function
- then call
flush()
It's easier to debug and refactor the flush call if its defined in one place.
Otherwise, there's going to be 3 instances of:
this.debouncedSubmitCustom(override ?? true);
this.debouncedSubmitCustom.flush();
For that reason, my vote is to keep flushPendingSubmitCustomUpdates
Overview
The
submitCustom
method can be called several times in rapid succession which leads to BE race conditions. To offset this we debounce the method so that each successive call has to wait 1 second.Notes
Testing
Submitted the
Book A Demo
feathery form before and after changes - works the same.