toss / es-toolkit

A modern JavaScript utility library that's 2-3 times faster and up to 97% smaller—a major upgrade to lodash.
https://es-toolkit.slash.page
Other
6.52k stars 291 forks source link

Confusing example for throttle function with { trailing: true } option #668

Open wondonghwi opened 1 week ago

wondonghwi commented 1 week ago

The documentation example for the throttle function when using the { trailing: true } option contains an incorrect comment. The comment states that the function is "logged immediately," which is inaccurate. When using trailing: true, the function should not be executed immediately but rather after the throttleMs delay period has passed since the last call.

image

Problematic Example:

const trailingFn = throttle(
  () => {
    console.log('Trailing function executed');
  },
  1000,
  { trailing: true }
);

// The comment below is incorrect:
console.log('Trailing function executed');

// Logs 'Trailing function executed' immediately
// Continuously calls will log 'Trailing function executed' every 1 second
trailingFn();

Correction:

const trailingFn = throttle(
  () => {
    console.log('Trailing function executed');
  },
  1000,
  { trailing: true }
);

// Should log after the throttle period, not immediately
// Logs 'Trailing function executed' after 1 second since the last call
trailingFn();

If this comment is not intended, may I modify the comment in both the English and Korean documentation to reflect the correct behavior?

raon0211 commented 1 week ago

Actually for throttle, the function should be invoked immediately after the first call. Is it not working in that way? Can you provide an example code?

wondonghwi commented 1 week ago

@raon0211 Thank you for your response.

Based on the documentation description of the edges option for throttle, I believe there may be some misunderstanding regarding the behavior of { trailing: true }.

As stated in the documentation:

If leading is included, the throttled function is executed immediately when first called. If trailing is included, the function is executed after the throttle delay (throttleMs) has passed since the last function call. Given this explanation, I believe the example comment may cause confusion. The current comment suggests that the function will be "logged immediately," but this is true only when { leading: true } is set. In the case of { trailing: true }, the function should only be executed after the delay, not immediately.

Could you please clarify if this is the intended behavior? If so, it would be helpful to update the comment in both the English and Korean documentation to prevent any further misunderstanding.

I appreciate your help in addressing this!

image
raon0211 commented 1 week ago

This issue arises from the way lodash handles its interface. In lodash, both leading and trailing default to true. So when you specify { trailing: true }, it actually implies { leading: true, trailing: true }. Since es-toolkit/compat aims for full compatibility with lodash, we follow its interface closely.

You can find this explained in our documentation, as shown in the screenshot below:

If you're using es-toolkit/compat, you'll need to set { leading: false, trailing: true } to achieve the intended behavior.

On the other hand, our original library, es-toolkit, avoids this confusion. We use throttle in a clearer way:

This eliminates any potential misunderstandings.

raon0211 commented 1 week ago

I guess the current example might be somewhat confusing, we might specify leading: false for the trailing example.

wondonghwi commented 1 week ago

@raon0211

Thank you for your response. I understand your explanation.

To ensure that the function doesn't execute on the first call but only after the throttle delay (throttleMs) has passed following the last call, we need to use { leading: false, trailing: true }.

Would it be okay if I added an example using { leading: false, trailing: true } to the documentation? I believe this would help clear up any potential confusion in the current explanation, and I would like to reflect this in the documentation.

raon0211 commented 1 week ago

Yes, sure! Thanks for your help.