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

Created debounce & throttle effects #27

Merged
merged 8 commits into from
Mar 4, 2018
Merged

Conversation

rmc
Copy link
Contributor

@rmc rmc commented Mar 1, 2018

I needed a debounce effect so I created one.

Closes #2.

@codecov
Copy link

codecov bot commented Mar 1, 2018

Codecov Report

Merging #27 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #27   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         116    134   +18     
  Branches       12     13    +1     
=====================================
+ Hits          116    134   +18
Impacted Files Coverage Δ
src/fxTypes.js 100% <100%> (ø) ⬆️
src/fxCreators.js 100% <100%> (ø) ⬆️
src/makeDefaultFx.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f44ac6c...074657e. Read the comment docs.

@okwolf
Copy link
Owner

okwolf commented Mar 2, 2018

Thanks for the PR. Have you taken a look at my proposal #2 for throttle? It seems very similar, except that inmediate (should be immediate btw) is always true. Do you have use cases where you always want the first time an action is called to be delayed? I would prefer delay over wait if we're going with debounce, but my real preference would be more of an action rate limiter/throttle with a prop of rate.

@rmc
Copy link
Contributor Author

rmc commented Mar 2, 2018

You're welcome. I had a look at #2 after I after I created this PR. I've prepared a throttle implementation, but I've not prepared the test yet.

throttleand debounce seem very similar, but the behaviour is subtly different. I've always liked this demo to visualise the difference.

Use case: listening for typing in a search ahead box. I don't want the search to start immediately, I want to start the search when the user has finished typing.

@okwolf
Copy link
Owner

okwolf commented Mar 2, 2018

@rmc thanks for the reference, I believe that confirms my beliefs WRT throttle vs debounce.

I hear folks mentioning the search box as a use case for debounce, but personally that user experience bugs me since you won't see the results update while you're typing. The ideal experience IMHO would be to trigger a search after each space immediately (more likely to match when you have a phrase with complete words) and throttle searches while the user is typing other characters so they don't overload your API.

Should we have both throttle and debounce effects? Or is there a way to write one effect that can do the work of both?

@rmc
Copy link
Contributor Author

rmc commented Mar 3, 2018

@okwolf I liked your suggestion for searching and was able to implement it with fxIf really easily (naive version):

    startSimpleSearch: event => fxIf([
      [/[^\s]$/.test(event.target.value), debounce(500, 'search')],  // last char is not whitespace
      [/\s$/.test(event.target.value), action('search')], // last char is whitespace
      [/^(?![\s\S])/.test(event.target.value), action('noop')] // had to add this for reasons I'll explain later
    ]

I'd go for adding both throttle and debounce. That way you could choose which you prefer according to your sensibilities/requirements. Writing one function to handle both cases would be complicated. Specifying the effect you would like to use explicitly makes more sense to me.

As for the last line in my example: I was implementing your search experience, and noticed that whenever all the conditions in fxIf evaluated to false, fxIf returns an empty array and the following error appears:

screen shot 2018-03-03 at 17 20 52

So I implemented a noop action, now whenever event.target.value is "" the error doesn't appear anymore. TBH I think I might be abusing fxIf. Am I right in thinking it always requires one condition to evaluate to true? I might be looking for and fxMaybe; execute all effects for which the condition evaluates to true? (This is a whole different issue though)

@okwolf
Copy link
Owner

okwolf commented Mar 3, 2018

@rmc thanks for trying out my crazy harebrained scheme!

I'm OK with adding both throttle and debounce since neither should be very large functions. How attached are you to having the immediate prop on debounce are you? I would think this feature wouldn't be used most of the time, so I'm thinking we should leave it out until someone really needs it. I'm also not a big fan of boolean arguments in general, so if we do find that we need such a thing, we might want to migrate to passing objects directly for the props to make it more like named arguments, but that's a different discussion...

I believe you are using fxIf correctly, but the code for running fx has a bug where it should treat empty arrays as a no-op effect instead of trying to run them. Like you said, that should be a different PR though. And that PR is #28.

@okwolf okwolf mentioned this pull request Mar 4, 2018
@okwolf okwolf added this to the 0.6 milestone Mar 4, 2018
@rmc
Copy link
Contributor Author

rmc commented Mar 4, 2018

Not really attached to immediate, removing it should simplify debouncea bit.

Should I attach the throttle effect to this PR as well?

Good to know I wasn't abusing fxIf too much ;-).

@okwolf
Copy link
Owner

okwolf commented Mar 4, 2018

@rmc Not really attached to immediate, removing it should simplify debouncea bit.

👍

@rmc Should I attach the throttle effect to this PR as well?

Your call, I'd like to get both in before making a new release either way. Also the bug fix for #28.

`Boolean` options are to be avoided. Can always be added back as a field on `props` if the need arises.
@okwolf
Copy link
Owner

okwolf commented Mar 4, 2018

@rmc thanks for the simplification!

I think we just need docs and this effect will be 💯.

@rmc rmc changed the title Created debounce effect Created debounce & throttle effects Mar 4, 2018
@rmc
Copy link
Contributor Author

rmc commented Mar 4, 2018

Docs are ready to go. Please have a look at them to make sure they're clear enough.

@okwolf okwolf merged commit 7436416 into okwolf:master Mar 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants