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

Changing debouncing example to make more sense #3650

Open
sisyfus opened this issue Jul 13, 2024 · 5 comments
Open

Changing debouncing example to make more sense #3650

sisyfus opened this issue Jul 13, 2024 · 5 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@sisyfus
Copy link

sisyfus commented Jul 13, 2024

IMHO the debouncing case study is not really very good at all, because it delays ALL requests - even the very first request!
https://riverpod.dev/docs/case_studies/cancel

When the user starts the app, the first http request should be sent immediately. Similarly, if the user HAS waited the requisite time after performing a refresh, the next refresh should of course be sent immediately. In the example, there will always be a delay from when the user initiates a new request, which IMHO is simply not how debouncing is supposed to work.

Would I be right in saying that it's not actually possible to implement proper debouncing without using state that is external to the provider? For example, if we were to start the timer AFTER a request, and then a dispose event occurred, we'd lose the timer state variable - correct?

If you don't intend to provide a more complete example, please at least consider adding a note, to advise the user that you are aware that it's not really a proper implementation.

@sisyfus sisyfus added documentation Improvements or additions to documentation needs triage labels Jul 13, 2024
@rrousselGit
Copy link
Owner

rrousselGit commented Jul 13, 2024

I think you're talking about throttling, not debouncing.

A debounce triggers the request after the no interaction has happened for "duration".
A throttle is like a cache. The operation triggers immediately, but nothing will happen again for "duration".

So the doc shows a real debounce, and you want something else.

@rrousselGit rrousselGit added question Further information is requested and removed needs triage labels Jul 13, 2024
@sisyfus
Copy link
Author

sisyfus commented Jul 13, 2024

Good point - thanks - I agree - I am indeed referring to throttling. Would you agree that throttling would make more sense for the sample app you give? E.g, if we increase the timeout to a large value, the user has to wait for a long time before the initial data is displayed. It just doesn't make sense, and it's confusing. I know these examples aren't supposed to be production quality, but IMHO they should at least make sense.

@sisyfus
Copy link
Author

sisyfus commented Jul 13, 2024

Actually, there's something else. If we accept that the example is indeed supposed to be demonstrating debouncing, shouldn't the app allow us to properly test it, by allowing the user to reset the timer by issuing a refresh whilst the previous one is pending? The app as it is only allows this for the very first request (i.e, immediately after the user has pressed the "+" action button on the homepage) - it correctly aborts the first request, resets the timer, and issues another request. Once on the details page though, the only way to issue a refresh is to do a "pull", but this doesn't do anything if there's a request pending, so strictly speaking, it's not actually performing debouncing once on the details page when doing pull-to-refresh - it's doing a sort of cross between debouncing & throttling.

@rrousselGit
Copy link
Owner

There are pros and cons.

Throttling may resolve with a value faster, but has the downside of possibly resolving with out-of-date state.
You wouldn't use throttling on a "search as we type" for instance.

Implementing throttling should be doable by storing the time when data was emitted in the state:

typedef State = ({DateTime time, Object? json});

@riverpod
class Example extends _$Example {
  @override
  Future<State> build() async {
    final current = DateTime.now();
    final previous = state.valueOrNull.time;
    if (/* not enough time elapsed between current and previous */) {
      return state.requireValue;
    }

    /* fetch */
  }
}

@sisyfus
Copy link
Author

sisyfus commented Jul 13, 2024

Thanks - appreciated.

I'm referring to the example app - I do not see any cons for using throttling in your example app.
I agree/understand that debouncing is the right thing for search-as-you-type.

Please consider changing the example to something that makes more sense for use with debouncing. ;^)

@rrousselGit rrousselGit changed the title Debounce case study suggestion: Add note that it's not really true debouncing - first request should occur immediately Changing debouncing example to make more sense Jul 13, 2024
@rrousselGit rrousselGit removed the question Further information is requested label Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants