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
fix(*): update useDebounce to fix delays in KTable after clearing the search #1220
Conversation
✅ Deploy Preview for kongponents ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
62fb1a0
to
fc9f48a
Compare
src/components/KCatalog/KCatalog.vue
Outdated
@@ -486,7 +486,9 @@ export default defineComponent({ | |||
hasInitialized.value = true | |||
} | |||
|
|||
const { query, search } = useDebounce('', 350) | |||
const query = ref('') | |||
const [search] = useDebounce((q: string) => { query.value = q }) |
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.
This appears to be a breaking change since the return format is now an array instead of an object?
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.
Yes. We only have two components using this composable: KTable and KCatalog. I intended to make useDebounce
return two functions: one is the denounced function, and the other one is a function that accepts a delay (other than the default one) and returns a new debounced function with that new delay (but which shares the same timeout handle). I tried to implement this composable to make it as general as possible for different circumstances.
This allows us to trigger the underlying function with a different delay. For example, if we want to call the function immediately and clear all pending calls, we can use the second function to generate a debounced function with no delay and trigger it.
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.
Is it possible to refactor the composable to return an object instead of an array; one property for each 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.
I refactored the useDebounce
composable to make it returns an object instead of an array.
src/components/KCatalog/KCatalog.vue
Outdated
@@ -486,7 +486,9 @@ export default defineComponent({ | |||
hasInitialized.value = true | |||
} | |||
|
|||
const { query, search } = useDebounce('', 350) | |||
const query = ref('') | |||
const [search] = useDebounce((q: string) => { query.value = q }) |
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.
Yes. We only have two components using this composable: KTable and KCatalog. I intended to make useDebounce
return two functions: one is the denounced function, and the other one is a function that accepts a delay (other than the default one) and returns a new debounced function with that new delay (but which shares the same timeout handle). I tried to implement this composable to make it as general as possible for different circumstances.
This allows us to trigger the underlying function with a different delay. For example, if we want to call the function immediately and clear all pending calls, we can use the second function to generate a debounced function with no delay and trigger it.
src/components/KTable/KTable.vue
Outdated
if (newValue === '') { | ||
search(newValue) | ||
} else { | ||
debouncedSearch(newValue) | ||
} |
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.
suggestion: Can you add some comments here to explain why we are using two different methods?
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.
Comments added
src/components/KTable/KTable.vue
Outdated
watch(() => [query.value, page.value, pageSize.value], ([newQuery, , , oldQuery]) => { | ||
if (newQuery === '' && newQuery !== oldQuery) { | ||
revalidate() | ||
} else { | ||
debouncedRevalidate() | ||
} | ||
}, { deep: true, immediate: true }) |
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.
This is a bit confusing with the empty array in the callback; could this be refactored or utilize watchEffect
instead?
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.
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.
Thanks. That's a good idea.
d746b2e
to
b0be109
Compare
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.
@sumimakito I reviewed everything here and it appears to be solid.
After merge, can you pull it into Kong Manager on Wednesday and test/verify all of the table fetchers are still working as intended? It's hard to test every use-case here
## [8.40.7](v8.40.6...v8.40.7) (2023-03-22) ### Bug Fixes * update useDebounce to fix delays in KTable after clearing the search ([#1220](#1220)) ([a550695](a550695))
🎉 This PR is included in version 8.40.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
After clearing the search input in a KTable, a perceptible delay is observed before KTable triggers the
fetcher
function.This PR also refactored the
useDebounce
function to accept functions, which makes it more general.Another option to fix this is to use the
defineExpose
to expose a function to clear the search input and trigger therevalidate
function immediately.KHCP-6651
PR Checklist