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

Complete support for leading, trailing, maxWait #62

Merged
merged 5 commits into from
Sep 1, 2020

Conversation

tryggvigy
Copy link

@tryggvigy tryggvigy commented Aug 31, 2020

This PR

This is a lot of changes to the source code but it was easier to start with lodash code and work backwards from there. Let me know what you think.

Things to note

  • The requestAnimationFrame is accessed via window.requestAnimationFrame. I'm not an experienced library author so I'm not sure if this is safe in all contexts (server side rendering, etc.). Looks like lodash uses https://github.com/lodash/lodash/blob/master/.internal/root.js to choose the right global object. I guess this is infamously painful in the JS ecosystem :)

this fixes jestjs/jest#2234
and allows us to properly mock the combination of
setTimeout and Date.now()
This matches the behaviour of lodash's debounce and uses
a lot of the lodash code.
@xnimorz
Copy link
Owner

xnimorz commented Aug 31, 2020

Great, thank you for the PR @tryggvigy!

At the moment, some unit tests have fallen, could you please check them? https://github.com/xnimorz/use-debounce/pull/62/checks?check_run_id=1053379621

I'll review this PR the next day (as it's 1 A.M. in my location)

@tryggvigy
Copy link
Author

Done, it's past midnight here as well 😄

Copy link
Owner

@xnimorz xnimorz left a comment

Choose a reason for hiding this comment

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

I don't have any additional notes except with the window variable. I'll checkout this branch, check if everything works correctly and cut a new version

isComponentUnmounted.current = true;
};
// Bypass `requestAnimationFrame` by explicitly setting `wait=0`.
const useRAF = !wait && wait !== 0 && typeof window.requestAnimationFrame === 'function';
Copy link
Owner

@xnimorz xnimorz Sep 1, 2020

Choose a reason for hiding this comment

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

When we call useDebouncedCallback during SSR, we could get ReferenceError: window is not defined, as we try to access requestAnimationFrame field in the undefined object.

I think this code could be improved, if the whole check will be the following:

const useRAF = !wait && wait !== 0 && typeof window !== 'undefined' && typeof window.requestAnimationFrame === 'function';

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I mentioned this in the PR description. Lodash uses an even more complicated check to resolve the global object but maybe keeping it simple like you suggest is enough. Not familiar with the pros/cons

@xnimorz
Copy link
Owner

xnimorz commented Sep 1, 2020

I'll merge and fix some problems with SSR. I'll write here when I release a new version.

@xnimorz xnimorz merged commit a5dd4b3 into xnimorz:master Sep 1, 2020
@xnimorz
Copy link
Owner

xnimorz commented Sep 1, 2020

Thank you for the PR, @tryggvigy

@tryggvigy
Copy link
Author

Nice, thanks @xnimorz

@tryggvigy tryggvigy deleted the complete-support-for-max-wait branch September 1, 2020 15:40
@xnimorz
Copy link
Owner

xnimorz commented Sep 2, 2020

Published use-debounce@4.0.0

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.

2 participants