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 before and after options, remove immediate option #9

Merged
merged 3 commits into from
Feb 19, 2020

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented May 10, 2019

Add before and after options, remove immediate option, fixes #5

Both before and after set to true lead to function which will never be executed and debounced version will always return undefined. Typed that for typescript, so should be easy to catch if it was a mistake, though don't see much sense in checking it at runtime.

Test before:false after:true options basically repeats what's tested in main debounces a function test, but still added it for clarity.

@sindresorhus sindresorhus changed the title Add before and after options, remove immediate option, fixes #5 Add before and after options, remove immediate option Jun 11, 2019
@sindresorhus
Copy link
Owner

Both before and after set to true lead to function which will never be executed and debounced version will always return undefined. Typed that for typescript, so should be easy to catch if it was a mistake, though don't see much sense in checking it at runtime.

Maybe we should follow the lodash.debounce behavior here?

Note: If leading and trailing options are true, func is invoked on the trailing edge of the timeout only if the debounced function is invoked more than once during the wait timeout. - https://lodash.com/docs/4.17.11#debounce

@sindresorhus
Copy link
Owner

// @bfred-it

@stroncium
Copy link
Contributor Author

  1. It is completely unrelated to current functionality(like make lowpass filter turn into highpass when some specific values are supplied)
  2. It prevents programmatic usage(That is, if you don't supply values directly but just pass them, you have to implement additional checks in order to ensure behavior won't change.)
  3. It can be implemented in much simpler way(one line, no need for any library).

@fregante
Copy link

fregante commented Jun 12, 2019

  1. Is it though? Debounce is specifically about multiple calls, so if both options are true I expect the first call to be instant and the following ones to wait. The unexpected behavior is not calling it at all, essentially making it a silent error.
  2. Maybe I’m misunderstand here, but are you describing your own solution? If we use lodash’ way of handling before/after = true, then the behavior follows what I’m telling it to do: call immediately and wait for successive calls. There’s no change in behavior from what I see.
  3. The implementation would not require lodash

Simply put, I generally trust lodash’ logic since the maintainer has been honing the library for years.

@stroncium
Copy link
Contributor Author

stroncium commented Jun 12, 2019

@bfred-it

  1. It's not always error. Also, not silent in typescript.

  2. I'm describing the situation when you don't hardcode values, but pass them from other source, possibly 2 unrelated sources. If the logic is inverted(as proposed, not exactly inverted, but close to it) once both values become true at once, you need to implement additional check to stop using debounceFn when both values are true. Forgetting to implement this check by user introduces silent and hard to debug error. Basically, all such invocations will be required to be like

let before = true; //saved from somewhere
const myDebouncer = (fn, after) => {
  //probably some logic here, what's important for us is just fn, before and after
  if (before && after) { //handle debounceFn changing behavior when both before and after are true
  	return () => undefined;
  }
  return debounceFn(fn, {before, after});
};
  1. You misunderstood me there, what I meant was proposed behavior is so simple to implement adhoc compared to learning details of the way some library(debounce-fn in this case) handles the task it starts to resemble is-true npm package. But even if it's a good addition, why not introduce a separate function for it?

@fregante
Copy link

fregante commented Jun 12, 2019

  1. That typescript catches it doesn’t mean that we can just discard any calls to the function for regular JS users. In no case should a call be silently discarded. When before/after are both false, then throw an error. If they’re both true, then follow both conditions.

  2. The “additional check” is only needed if the user wants to implement your non-standard behavior (the standard being lodash)

  3. I really don’t know what you’re talking about. How is what we described “so simple to implement ad-hoc”? I disagree. Can you show such example? Maybe there’s some fundamental misunderstanding.

debounce-fn itself is “so simple to implement ad-hoc” but we still justify the existence of this module.

@sindresorhus
Copy link
Owner

I agree with what @bfred-it said above.

@stroncium
Copy link
Contributor Author

@sindresorhus ping

Comment on lines +9 to +10
const before = (options.before === undefined) ? false : options.before;
const after = (options.after === undefined) ? true : options.after;

Choose a reason for hiding this comment

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

I think it's worth using a regular Object.assign or {...} to define defaults and destructure all the options in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a big fan of memory allocations without any purpose, but since it's hard to expect this method to be used frequently, we can add allocation I think.

Copy link

Choose a reason for hiding this comment

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

You could also use destructuring with defaults in the function parameters so no new objects would be created

@sindresorhus sindresorhus merged commit 689f847 into sindresorhus:master Feb 19, 2020
@sindresorhus
Copy link
Owner

Thanks :)

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.

Allow both leading and trailing
3 participants