Skip to content
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

feat(ui): Add useDebouncedValue hook #55726

Closed
wants to merge 1 commit into from

Conversation

davidenwang
Copy link
Contributor

@davidenwang davidenwang commented Sep 5, 2023

Useful hook for having a value which has debounced updates.

Example usage in:
#55730

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Sep 5, 2023
@davidenwang davidenwang marked this pull request as ready for review September 7, 2023 17:57
@davidenwang davidenwang requested a review from a team September 7, 2023 17:57
* Given value and delay in ms, returns a version of that value which receives debounced updates
* according to the specified delay
*/
export function useDebouncedValue<T>(value: T, delay: number): T {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to just debounce setState calls? I have a hard time imagining what the use-case would be for a hook like this?

For example, I would imagine the usage for the debouncedSetState calls to be

const [debouncedState, setDebouncedState] = useDebouncedState(initialValue, delay)

where calling setDebouncedState is a debounced wrapper around setState.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the example use case that I'd want this hook for #55730, for that example I want to debounce updates to a non-state value so it makes more sense there.

I definitely see the usefulness of a useDebouncedState but it would add more complexity to my use case, and I'm not sure which other parts of the app need a useDebouncedState right now.

If you still would prefer it though I could switch to useDebouncedState and just add a little more logic in my use-case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I'm understanding this better. I wonder though, since you rely on the value to change in order for your hook to update, it means it must be stored as state somewhere (else it would never fire), meaning you are just opting out of rerenders in a specific part of the tree here? Is my understanding correct? The right way to opt out of rerenders in react is to not modify the state at all so that you don't end up rerendering everything in the first place and don't have to selectively opt-out down the line.

tbh, I don't mind hooks like these, I'm sure there are probably valid use cases, but it feels to me like they are the wrong solution to the problem and shouldn't be used often. I def wouldn't be comfortable if everyone starts reaching for this hook vs debouncing calls to setState itself

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized something reading your PR in https://github.com/getsentry/sentry/pull/55730/files, but knowing that sentry has DEFAULT_DEBOUNCE_DURATION const in sentry/constants requires codebase knowledge/context that most folks don't have (especially folks new to the codebase), meaning they are likely going to reach for custom debounce values instead of importing it, which means we'll end up with subtly different values all over the place.

I would either make DEFAULT_DEBOUNCE_DURATION a default arg value in this hook (preferred) or reexport it from useDebouncedValue so that anyone looking at the useDebouncedValue has a higher chance of seeing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh sorry for the late reply here, and yes you are correct I'm opting out of set state calls from our useDimensions hook because as the user resizes their browser window the hook will fire many calls to setState triggering the rollup value in https://github.com/getsentry/sentry/pull/55730/files to change and re-trigger a network call.

I follow your argument and agree that a hook to debounce setState calls does feel better. So I might actually just not add this as a top level hook in our /utils and opt to make a utility function/hook for my specific use case.

@getsantry
Copy link
Contributor

getsantry bot commented Oct 19, 2023

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Oct 19, 2023
@getsantry getsantry bot closed this Oct 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants