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

throttleImmediate #702

Open
Phoscur opened this issue Jan 7, 2018 · 12 comments
Open

throttleImmediate #702

Phoscur opened this issue Jan 7, 2018 · 12 comments

Comments

@Phoscur
Copy link

Phoscur commented Jan 7, 2018

I'd like to filter my stream to issue update events (rerequesting resources) so I want to debounceImmediate but then just to be sure, issue another update if something changed in the debounce period.
Example:

var throttled = source.throttleImmediate(2)
source:    asdf----asdf----a--
throttled: a-d-f---a-d-f---a--

How would you accomplish this currently? With a custom bacon operator?

@semmel
Copy link
Member

semmel commented Jan 16, 2018

Isn't simply Bacon.mergeAll(source.debounceImmediate(2), source.debounce(2)) ?

var source = Bacon.sequentially(250, 'asdfghjk'.split('')).concat(Bacon.sequentially(250, 'asdfghjk'.split('')).delay(5000));
var throttled = Bacon.mergeAll(testSeq.debounceImmediate(990), testSeq.debounce(990));
source.log(':'); throttled.log('t:');
// asdfghjk_____asdfghjk   
// a___g___k____a___g___k

@Phoscur
Copy link
Author

Phoscur commented Jan 16, 2018

Merging debounceImmediate and debounce gives us throttleImmediate - seems like the right solution.
Thanks @semmel !

Reopen if you would like to add this to the Bacon API.

@Phoscur Phoscur closed this as completed Jan 16, 2018
@raimohanska raimohanska reopened this Jan 17, 2018
@raimohanska
Copy link
Contributor

I think it would make sense to add into the API. Pull Request anyone?

@Phoscur
Copy link
Author

Phoscur commented Jan 31, 2018

@semmel the merging produces two events in case only one was fired, we might need some more code here to prevent this.

It is supposed to only fire a second event, if there was one in the debounce period.

@Phoscur
Copy link
Author

Phoscur commented Feb 10, 2018

@raimohanska @semmel any help concerning this would be very appreciated as this has become an urgent issue for me. I seem to be still not familiar enough with reactive programming or I just have some thought block on this.

@Phoscur
Copy link
Author

Phoscur commented Feb 10, 2018

The current failing test could be trivially solved by filtering duplicates out. But how does that work on more complex event objects, an isEqual comparator would need to be supplied?

@Phoscur
Copy link
Author

Phoscur commented Feb 12, 2018

Resolved urgency of the issue by optimizing input and update processing at the end of the stream. Events are more detailed now.

Still considering to just debounce the eventstream, possibly with a partially random time interval to stagger downloads a bit, what do you think, do you need more information on the usecase?

@raimohanska
Copy link
Contributor

raimohanska commented Feb 12, 2018

Maybe this:

Observable.prototype.throttleImmediate = (delay) => 
  this.flatMapFirst(first => B.once(first).concat(B.later(delay).filter(false)))

Might be worth trying?

@Phoscur
Copy link
Author

Phoscur commented Feb 12, 2018

@raimohanska as you can see in #712, I already tried to create a debounceImmediate variant and borrowed from the tests.

debounceImmediate does not solve the usecase:
Using a high delay, e.g. one Minute, all subsequent Events in the delay will be missed, and the data returned from the "immediate" request, will be outdated.

@raimohanska
Copy link
Contributor

Oh, you want to include the last event in the batch too. Should have looked at your diagram up there.

@raimohanska
Copy link
Contributor

raimohanska commented Feb 12, 2018

How about this: https://codesandbox.io/s/o91z082j5y

Got a bit complicated, which is disappointing. Are we missing some key methods that would make this easier?

@Phoscur
Copy link
Author

Phoscur commented Feb 14, 2018

Before I saw your PR, I added a shortened version of that sandbox code to my PR #712. It works and covers EventStream tests. I was able to leave out the part merging the sampler with the silence..

Properties still output a duplicate though. @raimohanska can you explain why?

Also the doubleEdgedDebounce of which you forked the sandbox from would suit my needs - why are they so different?

How would you like to land one of these doubleEdged implementations?

The leadingThrottle implementation works for my usecase, but it doesn't skip events as I first intended.

@Phoscur Phoscur closed this as completed Feb 14, 2018
@Phoscur Phoscur reopened this Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants