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 ability to debounce window resize updates #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

camflan
Copy link

@camflan camflan commented Oct 26, 2018

This allows a user to pass a debounceMs when calling useWindowSize in order to debounce the window resize events

This allows a user to pass a debounceMs when calling useWindowSize in order to debounce the window resize events
@jamiebuilds
Copy link
Member

please don't include formatting changes with your PR

@jamiebuilds
Copy link
Member

you're still changing the formatting with indentation

@camflan
Copy link
Author

camflan commented Oct 26, 2018

@jamiebuilds I'm working on it :)

It certainly wasn't my intention to change your formatting. the first commit was through code sandbox and it applied prettier automatically

@camflan
Copy link
Author

camflan commented Oct 26, 2018

any objection to adding a prettier config to maintain your formatting? I'm happy to get one set up in another PR

@jamiebuilds
Copy link
Member

Sure, could you submit a PR to https://github.com/rehooks/template

@jamesb3ll
Copy link

What about receiving the debounce function as an argument?

import { debounce } from 'lodash/fp';
function App() {
  let windowWidth = useWindowWidth();
  let debouncedWindowWidth = useWindowWidth(debounce(500))
    return [
        <pre>Debounced: {JSON.stringify(debouncedWindowWidth)}</pre>,
        <pre>Firehose: {JSON.stringify(windowWidth)}</pre>,
    ]
}

@camflan
Copy link
Author

camflan commented Oct 29, 2018

@jamesb3ll

That could be pretty cool to allow a user to pass a function to apply to the event handler before updating state, then they could pick throttle, debounce, filter, etc, etc. However, I'm not sure lodash/debounce works like that. I think debounce takes a fn as the first arg, delay as second.

We could specify that they give us a function that will take our fn as an arg? Kind of wonky though.

import { debounce as _debounce, throttle as _throttle }  from 'lodash/fp';

let debounce = delay => fn => _debounce(fn, delay)
let throttle = delay => fn => _throttle(fn, delay)

function App() {
  let windowWidth = useWindowWidth();
  let debouncedWindowWidth = useWindowWidth(debounce(500))
  let throttledWindowWidth = useWindowWidth(throttle(500))


    return [
        <pre>Debounced: {JSON.stringify(debouncedWindowWidth)}</pre>,
        <pre>Throttled: {JSON.stringify(throttledWindowWidth)}</pre>,
        <pre>Firehose: {JSON.stringify(windowWidth)}</pre>,
    ]
}

🤷‍♂️

Is there a better way?

@jamesb3ll
Copy link

@camflan the lodash/fp package is for functional programming and should be data last and currying for arguments. No changes should be needed :)

@jedwards1211
Copy link

jedwards1211 commented Mar 2, 2019

@camflan @jamesb3ll If you pass a function argument to useWindowWidth you need to memoize the function...otherwise it's different on every render and useWindowWidth has to re-throttle/re-debounce each time.

I mean I guess you could only use the first function ever passed, but that might violate some users' assumption that they can pass a function with a different delay to change throttle/debounce rate.

@jedwards1211
Copy link

@camflan to elaborate on what @jamesb3ll said, AFAIK debounce from lodash/fp already has the signature delay => fn => throttledFn. It's the non-fp version that has the signature (fn, delay) => throttledFn.
I can't see any other way, at first I thinking a separate hook that wraps the base hook should perform the throttling, but then I realized, even if setWindowSize is throttled, the component will still rerender on every event.

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

Successfully merging this pull request may close these issues.

4 participants