- Notifications
You must be signed in to change notification settings - Fork947
fix(site): speed up state syncs and validate input for debounce hook logic#18877
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| value: T, | ||
| debounceTimeMs: number, | ||
| ): T { | ||
| export function useDebouncedValue<T>(value: T, debounceTimeoutMs: number): T { |
ParkreinerJul 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Removed the default parameter here because if you, the user of the hook, don't know the type of a value you're trying to debounce, you're probably doing somethingreally wrong
| ); | ||
| } | ||
| const timeoutIdRef = useRef<number | undefined>(undefined); |
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.
Updated ref type to match the type used by the browser'ssetTimeout
| @@ -12,7 +12,7 @@ afterAll(() => { | |||
| }); | |||
| describe(`${useDebouncedValue.name}`, () => { | |||
| function renderDebouncedValue<T = unknown>(value: T, time: number) { | |||
| function renderDebouncedValue<T>(value: T, time: number) { | |||
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.
Default type parameter removed to match new type signature for the main hook
site/src/hooks/debounce.test.ts Outdated
| @@ -12,7 +12,7 @@ afterAll(() => { | |||
| }); | |||
| describe(`${useDebouncedValue.name}`, () => { | |||
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.
| describe(`${useDebouncedValue.name}`,()=>{ | |
| describe(useDebouncedValue.name,()=>{ |
I know you didn't change this line, but just noticed
site/src/hooks/debounce.ts Outdated
| ): UseDebouncedFunctionReturn<Args> { | ||
| if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { | ||
| throw new Error( | ||
| `Provided debounce value ${debounceTimeoutMs} must be a non-negative integer`, |
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.
Provided debounce value -1 must be a non-negative integer
I'm not sure this wording makes much sense. how about something like...
| `Provided debouncevalue${debounceTimeoutMs}must bea non-negative integer`, | |
| `Invalidvalue${debounceTimeoutMS} for debounceTimeoutMs. Valuemust begreater than or equal to zero.`, |
site/src/hooks/debounce.ts Outdated
| export function useDebouncedValue<T>(value: T, debounceTimeoutMs: number): T { | ||
| if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { | ||
| throw new Error( | ||
| `Provided debounce value ${debounceTimeoutMs} must be a non-negative integer`, |
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.
Same thing here
f47efc6 intomainUh oh!
There was an error while loading.Please reload this page.
No issue to link – I'm basically pushing some updates upstream from the version of the hook I copied over for the Registry website.
Changes made
useDebouncedValueto flush state syncs immediately if timeout value is0