Add leading and trailing options for debounce helper #2380 #2382
Conversation
|
There was a problem hiding this comment.
Thanks for submitting this PR! I had one comment about the API and then we also need to ensure our types are updated to reflect this change.
See here for a starting place: https://github.com/redux-saga/redux-saga/blob/main/packages/core/types/ts3.6/effects.d.ts#L1220
| @@ -1,8 +1,16 @@ | |||
| import fsmIterator, { safeName } from './fsmIterator' | |||
| import { delay, fork, race, take } from '../io' | |||
|
|
|||
| export default function debounceHelper(delayLength, patternOrChannel, worker, ...args) { | |||
| let action, raceOutput | |||
| export default function debounceHelper(delayLengthOrOptions, patternOrChannel, worker, ...args) { | |||
There was a problem hiding this comment.
What happens if both leading and trailing are set to true?
There was a problem hiding this comment.
The saga will run at the start of the wait period, and if any further actions occur during the wait period, it will run again at the end of the wait period. This matches the behavior of the lodash debounce function with leading: true, trailing: true. I imagine this will be a common use-case - it's how I will use it personally.
There is an example of this in the tests.
There was a problem hiding this comment.
I have deployed a little example here https://heartfelt-semifreddo-a4737f.netlify.app/
(Hit any key on the keyboard to see the effect of a 1000ms debounce with the various options).
Source code is here
|
@neurosnap I just wanted to gently bump this - do you have any concerns about this change? Or are there any further changes required? |
|
Thanks for the bump. I haven't forgotten about this PR, just need to make time to fully review and test it. I'll try to get this reviewed by end of week. |
There was a problem hiding this comment.
We still need types updated for https://github.com/redux-saga/redux-saga/blob/main/packages/core/types/ts3.6/effects.d.ts#L1220
| leading = false | ||
| trailing = true | ||
| } else { | ||
| ;({ delayLength, leading = false, trailing = true } = delayLengthOrOptions) |
There was a problem hiding this comment.
This syntax reads a little awkward, could we rework this?
This adds
leadingandtrailingoptions to thedebouncehelper, as suggested in the linked issue. The first parameter todebouncecan now be an options object of format{ delayLength: number; leading?: boolean; trailing?: boolean }, or for backwards compatibility can still just be anumberspecifyingdelayLength.leadingdefaults tofalseandtrailingdefaults totrue. The behaviour in this case should be unchanged compared to onmain.The behaviour of "leading" and "trailing" should essentially match how it works in lodash.