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: create debounce and debounce first transformers #4268

Closed
wants to merge 14 commits into from

Conversation

mrgnhnt96
Copy link

@mrgnhnt96 mrgnhnt96 commented Oct 26, 2024

Status

READY

Breaking Changes

NO

Description

This PR introduces a new transformer:

Debounce

This is the traditional debounce transformer. When any event is received, a "quiet period" will be created, based on the duration provided. When the window completes, the last event received will be processed.

# duration = 100ms
Event 1 -- 10ms -- Event 2 -- 10ms -- Event 3
  • Event 1: debounce
  • Event 2: debounce
  • Event 3: process

Leading

When leading = true, then the first event will not be debounced, rather processed. The "quiet period" will still be created and the last received event will still be processed.

# duration = 100ms
Event 1 -- 10ms -- Event 2 -- 10ms -- Event 3
  • Event 1: process
  • Event 2: debounce
  • Event 3: process

Transformer

After the "quiet period" is complete, it's possible that the previous event hasn't resolved yet, necessitating a second transformer to handle the events to process. The concurrent transformer is the default here.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.

Project coverage is 99.52%. Comparing base (d1abf0f) to head (1dabb35).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
packages/bloc_concurrency/lib/src/debounce.dart 88.23% 2 Missing ⚠️
...kages/bloc_concurrency/lib/src/debounce_first.dart 89.47% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master    #4268      +/-   ##
===========================================
- Coverage   100.00%   99.52%   -0.48%     
===========================================
  Files           31       33       +2     
  Lines          803      839      +36     
===========================================
+ Hits           803      835      +32     
- Misses           0        4       +4     
Flag Coverage Δ
bloc_concurrency 93.44% <88.88%> (-6.56%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@felangel felangel added enhancement New feature or request pkg:bloc_concurrency This issue is related to the bloc_concurrency package labels Oct 28, 2024

/// {@macro debounce}
EventTransformer<E> debounce<E>([
Duration duration = const Duration(milliseconds: 300),
Copy link
Owner

Choose a reason for hiding this comment

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

I think it'd be nice to have the duration be a required, named argument and to align with the API from package:stream_transform. In fact, we already depend on package:stream_transform so I think we should be able to dramatically simplify the implementation here.

I'm thinking the method signature could look something like:

EventTransformer<E> debounce<E>({
  required Duration duration,
  bool leading = false,
  bool trailing = true,
}) {...}

This would also allow us to remove debounceFirst altogether and there'd just be a single debounce transformer.

Copy link
Owner

@felangel felangel Oct 30, 2024

Choose a reason for hiding this comment

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

I'm also just remembering why I never included debounce to begin with -- because it's purely rate limiting the event stream but doesn't indicate how the emitted events are processed (e.g. are they still processed concurrently, sequentially, etc?).

In other words, let's say you apply a debounce(10ms) and we have something like:

Event 1 -- 10ms -- Event 2 -- 10ms -- Event 3

And let's say Event 1 takes 30ms to process so by the time Event 2 is added, Event 1 still hasn't finished processing what should the behavior be? This is why I feel debounce doesn't quite fit here unless it also accepts a converter function that allows the user to define how the debounced events should be processed.

@felangel
Copy link
Owner

felangel commented Oct 30, 2024

Left a comment based on my recollection and I think it'd be helpful to better understand the problem we're solving here. Can you maybe describe some real-world use-cases that would benefit from a debounce transformer where droppable and/or restartable won't help? Thanks!

It's also worth nothing that you can already trivially apply a debounce with any of the existing transformers like:

import 'package:bloc/bloc.dart';
import 'package:bloc_concurrency/bloc_concurrency.dart';
import 'package:stream_transform/stream_transform.dart';

/// Debounce events and the process the emitted events concurrently.
EventTransformer<E> concurrentDebounce<E>({
  required Duration duration,
  bool leading = false,
  bool trailing = true,
}) {
  return (events, mapper) {
    return concurrent<E>().call(
      events.debounce(duration, leading: leading, trailing: trailing),
      mapper,
    );
  };
}

/// Debounce events and the process the emitted events sequentially.
EventTransformer<E> sequentialDebounce<E>({
  required Duration duration,
  bool leading = false,
  bool trailing = true,
}) {
  return (events, mapper) {
    return sequential<E>().call(
      events.debounce(duration, leading: leading, trailing: trailing),
      mapper,
    );
  };
}

Hopefully that gives you a better sense of why a debounce implementation was not included.

@felangel felangel added enhancement candidate Candidate for enhancement but additional research is needed waiting for response Waiting for follow up and removed enhancement New feature or request labels Oct 30, 2024
@mrgnhnt96
Copy link
Author

mrgnhnt96 commented Oct 30, 2024

Thank you for the feedback!

The main real-world use-case for a debounce transformer is to handle user interactions that trigger frequent events, such as search inputs or scrolling. These situations can quickly flood the event stream, and debounce helps limit the number of events processed. Specifically, droppable and restartable might not be suitable here because they either drop intermediate events or restart processing, while debounce simply defers processing until a certain “window period" is reached. Debounce would mean that only the final value after a burst of events is processed, without dropping or restarting events.

I see your point that debounce could already be implemented using the existing transformers along with stream_transform. The goal with this PR was to simplify the API and provide a ready-made, cohesive solution directly within bloc_concurrency, making it more accessible for developers who may want this common behavior without having to build their own transformer each time.

Thanks for suggesting the changes to the method signature, like making duration required and matching the stream_transform API. It makes sense to simplify things by using stream_transform, so I'll update it.

@mrgnhnt96
Copy link
Author

Just pushed my changes, wanted to note on a few things:

  1. I used concurrent as the default transformer to handle events to be processed within the debounce transformer. I felt this was fitting as the default transformer for bloc is concurrent

  2. I didn't include the trailing parameter in the signature for debounce. This is because if leading = true and trailing = false, then essentially we've recreated the droppable transformer. LMK if you think that we should still include this and I'll revert this commit

@felangel
Copy link
Owner

felangel commented Nov 4, 2024

Just pushed my changes, wanted to note on a few things:

  1. I used concurrent as the default transformer to handle events to be processed within the debounce transformer. I felt this was fitting as the default transformer for bloc is concurrent
  2. I didn't include the trailing parameter in the signature for debounce. This is because if leading = true and trailing = false, then essentially we've recreated the droppable transformer. LMK if you think that we should still include this and I'll revert this commit

Thanks for all the hard work. The problem I'm having is, as I mentioned, debounce is not really an event transformer.

An event transformer = an optional event filter + a required event mapper.

Debounce is just an event filter (it doesn't imply anything about how the debounced events will be processed). In addition, it's trivial for folks to create their own custom debouncing transformer:

import 'package:bloc/bloc.dart';
import 'package:bloc_concurrency/bloc_concurrency.dart';
import 'package:stream_transform/stream_transform.dart';

EventTransformer<E> concurrentDebounce<E>({
  required Duration duration,
  bool leading = false,
  bool trailing = true,
}) {
  return (events, mapper) {
    return concurrent<E>().call(
      events.debounce(duration, leading: leading, trailing: trailing),
      mapper,
    );
  };
}

EventTransformer<E> sequentialDebounce<E>({
  required Duration duration,
  bool leading = false,
  bool trailing = true,
}) {
  return (events, mapper) {
    return sequential<E>().call(
      events.debounce(duration, leading: leading, trailing: trailing),
      mapper,
    );
  };
}

It seems like the bigger issue here is the lack of advanced docs on writing event transformers which I am happy to work on.

For now though, I don't think it makes sense for a debounce event transformer to be included because by definition debounce is only part of an event transformer (without a mapper it's just a filter).

Hope that makes sense and sorry for leading you down this rabbit hole -- I totally forgot why I didn't do this in the first place!

Closing for now but happy to continue the discussion 👍

@felangel felangel closed this Nov 4, 2024
@felangel felangel removed the waiting for response Waiting for follow up label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement candidate Candidate for enhancement but additional research is needed pkg:bloc_concurrency This issue is related to the bloc_concurrency package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants