-
Notifications
You must be signed in to change notification settings - Fork 948
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
value: T, | ||
debounceTimeMs: number, | ||
): T { | ||
export function useDebouncedValue<T>(value: T, debounceTimeoutMs: number): T { |
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 something really 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's setTimeout
@@ -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 debounce value ${debounceTimeoutMs} must be a non-negative integer`, | |
`Invalid value ${debounceTimeoutMS} for debounceTimeoutMs. Value must be greater 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
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
useDebouncedValue
to flush state syncs immediately if timeout value is0