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

Add debounce capabilities #6147

Closed
drewdaemon opened this issue Aug 18, 2022 · 10 comments
Closed

Add debounce capabilities #6147

drewdaemon opened this issue Aug 18, 2022 · 10 comments

Comments

@drewdaemon
Copy link

Debouncing can improve the UX significantly when input events trigger an automatic operation such as an expensive render. This is the case in many places in Kibana.

In Lens we make heavy use of a useDebouncedValue hook. It is used like this:

const onChange = useCallback(() => { ... });

const { inputValue, handleInputChange } = useDebouncedValue(
    {
      onChange,
      value,
      defaultValue,
    },
    { allowFalsyValue: true }
  );

  return (
    <EuiFieldText
      value={inputValue}
      onChange={(e) => {
        handleInputChange(e.target.value);
      }}
    />
  );

We also recently introduced a DebouncedInput component which sacrifices some flexibility to make this even easier:

<DebouncedInput
  value={myVal}
  onChange={(newVal) => {
    // state update
  }}
/>;

These helpers could form inspiration/a starting point for the capabilities in EUI. This feels like a good fit for the EUI library so that this generally-useful feature can be available everywhere.

@drewdaemon drewdaemon added feature request ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible labels Aug 18, 2022
@chandlerprall
Copy link
Contributor

Similarly, we have a useDebouncedUpdate in our docs' utils, used in a couple examples.

There are other community offerings providing this, including https://usehooks.com/useDebounce/ and https://www.npmjs.com/package/@react-hook/debounce, all of these have their differences in UX and DX.

As part of triaging this we would want to understand the use cases that need to be covered and why existing implementations aren't sufficient. Alternatively, we could recommend a particular package (and consume it in our examples).

@flash1293
Copy link
Contributor

flash1293 commented Aug 18, 2022

Maybe debounced value isn't the right name, it's doing more than the ones you linked. It's basically like an optimistic prop updater, keeping a local mirror for immediate UI feedback.

Think of it like this. Very often in a component you get props like this:

({ value, setValue }) => <>
  // render value and input to update it
</>

The value comes from someplace upstream, maybe a state hook in a parent component or a redux store somewhere.

You know, if setState is called with a new value, the component will be re-rendered with the updated value. The thing is if you just call setValue on every onChange of an input element, then the input might be very laggy because that callback might go really far up before coming back down (think passing through three layers of wrapping callbacks before being passed into a reducer with four middlewares kicking off async work, then passing through five chained selectors before being passed down six layers through the component tree, re-rendering seven other components in the process because they are not memoized properly - you get the idea). If enough stuff is happening in this loop, it might take > 16ms for the new value to actually appear in the input which will feel extremely laggy.

Debouncing is the common way to work around this issue - keep a local state of what the user is typing and only send it upstream if they stop typing for some time. In most places that works, because the input the user is typing in is the only place in the UI the user can change the value and the linked helpers can assist with that. However, what happens if it's possible to update the state from somewhere else as well (e.g. by an incoming websocket package, or by clicking somewhere else in the UI and it also changes the value synced with that input - that latter is what happened in the Lens case)? The input won't update with the new value because it already set its local state on first render. Simply updating the local state whenever a new value arrives from upstream doesn't work either, because they might collide with local changes being produced while waiting for the upstream state to update (e.g. user types "abc", then stops - upstream callback is called with "abc". Before upstream state management had a chance to update and pass down the new props, the user started typing again, typing "def", then stops. Now the callback is called with "abcdef". Afterwards, the new state arrives from upstream and resets the input to "abc". A bit later, upstream state has handled the "abcdef" update and the input updates again back to "abcdef".

The Lens useDebouncedValue hook is handling this case in the following way:

  • It keeps track of the upstream state passed down as well as the local state
  • It also keeps a dirty flag whether the current local state did not get sent upstream yet via the callback
  • If an updated value arrives from upstream, it is accepted if there is no unflushed local value - it's a regular state update caused by something else in the system
  • If there is an unflushed value around though, the newly arrived value from upstream isn't relevant because we will override it in a second with the changes we are going to call the upstream callback with. So we ignore this incoming value and keep showing the user what they have in their local state so far

Hope that makes sense, happy to discuss

@flash1293
Copy link
Contributor

flash1293 commented Aug 18, 2022

Maybe important to note that this method is not preventing conflicts in 100% of the cases, but it has worked well for our use case of smooth optimistic update with a lagging global state that might take a few hundred milliseconds to update, but not multiple seconds.

@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@drewdaemon
Copy link
Author

I believe this is still valid.

@cee-chen cee-chen removed the ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible label May 1, 2023
@JasonStoltz
Copy link
Member

JasonStoltz commented May 2, 2023

It seems like there are two parts to this:

  1. Laggy state updates can cause UI to lag in controlled components.

    As you mention, this is not too uncommon and folks typically work around this by keeping "a local state of what the user is typing and only send it upstream if they stop typing for some time" (the debounce approach)

    I also wonder if this could be handled by simply using the component as an uncontrolled component rather than a controlled component.

  2. Working with the debounce approach can cause conflicts when the value may also change upstream from other sources

    Your debounce work adds additional checks to handle properly sequencing these events.

    This piece gives me the most hesitation. It adds a lot of complexity to the data flow in forms and I'd worry about the side effects of overusing this pattern. For that reason, I'd be especially hesitant to add it as a utility to EUI.

How big is the need for this pattern? Is this a generally useful utility or is this more of an edge case specific to Lens?

Before we consider this, I think I'm looking for the following information:

  1. Validation that this is, in fact, a generally useful utility that would benefit Kibana developers and/or generally make the UI developer experience better in Kibana.
  2. Validation that there isn't a cleaner approach to solving this problem

@JasonStoltz
Copy link
Member

JasonStoltz commented May 3, 2023

The other thing that is top of mind here for me -- is this really a UI concern, or a higher-level state management concern?

@drewdaemon
Copy link
Author

drewdaemon commented Jun 2, 2023

Thanks for the response.

No strong feelings here. It felt like a fundamental UI-related utility so we discussed pushing it upstream to EUI. But, I see your point about it also being related to state management.

Feels like it solves a problem that any app using redux can run into, but you're right that it could add complexity and be overused.

Validation that this is, in fact, a generally useful utility that would benefit Kibana developers and/or generally make the UI developer experience better in Kibana.

I'm not sure where this would come from. I don't have much to offer other than the Lens anecdote.

Validation that there isn't a cleaner approach to solving this problem

I agree with you that using uncontrolled components should work well if there isn't a possibility of the bound state changing from another place in the app, but I'm not sure what a cleaner solution would look like if it can (open for ideas).

No pressure from me to keep the issue open.

Copy link

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

drewdaemon added a commit to elastic/kibana that referenced this issue Jan 26, 2024
## Summary

Fix #174653

The bug that was causing the failures


https://github.com/elastic/kibana/assets/315764/058a99cd-b7a6-4e3b-9f99-208b2712136a



I fixed it by divorcing the state of the annotation controls from the
redux store. Redux is still notified with changes to the annotation
group, but the UI no longer waits on the global state. That makes things
faster and resolved this state management weirdness.

I feel that we should consider a similar approach with the rest of Lens
(see elastic/eui#6147 (comment)
for more discussion).

Passing flaky test runner
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4895

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
## Summary

Fix elastic#174653

The bug that was causing the failures


https://github.com/elastic/kibana/assets/315764/058a99cd-b7a6-4e3b-9f99-208b2712136a



I fixed it by divorcing the state of the annotation controls from the
redux store. Redux is still notified with changes to the annotation
group, but the UI no longer waits on the global state. That makes things
faster and resolved this state management weirdness.

I feel that we should consider a similar approach with the rest of Lens
(see elastic/eui#6147 (comment)
for more discussion).

Passing flaky test runner
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4895

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Copy link

❌ Per our previous message, this issue is auto-closing after having been open and inactive for a year. If you strongly feel this is still a high-priority issue, or are interested in contributing, please leave a comment or open a new issue linking to this one for context.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants