jashkenas / underscore

JavaScript's utility _ belt
https://underscorejs.org
MIT License
27.29k stars 5.53k forks source link

Run trailing debounce when browser tab is closed #2984

Closed h closed 1 year ago

h commented 1 year ago

A common issue with debounced API calls is that when the user closes the browser tab, the debounced function may not have run yet. This is a frequent cause of data loss. The debounce implementation now prevents this issue by running the trailing debounce immediately if the tab is closed prior to the trailing timeout.

The code detects whether it's running in an environment with access to the DOM. In runtimes where this function is unavailable (e.g. Node.js) the new functionality is ignored since would not be relevant in that context.

Resolves #2983 @jashkenas

jgonggrijp commented 1 year ago

I appreciate your willingness to contribute, but unfortunately, I have to reject this change. There are two main reason for rejecting.

Firstly, changes of this type are highly problematic. The implementation of debounce and throttle is extremely tricky to get right, as shown by the large number of changes that went into them and the large number of tests dedicated to just those two functions. At the same time, literally millions of developers rely on these functions working correctly every day. Unless the behavior of debounce is clearly broken, which is not the case here, I cannot justify risking new bugs by making "feature additions" that are arguably somewhat arbitrary. The use case you're trying to address is extremely specific, and I'm not even fully convinced that your suggested changes will address it completely.

Secondly, you can realize the suggested behavior without changing the implementation of debounce at all. Instead of using a debounced function directly as an event handler, like in the example below,

var handler = _.debounce(expensiveAPICall);
document.querySelector('#button').addEventListener(handler);

you can define a new, reusable wrapper function that manages a debounced version of the callback as well as a visibilitychange event handler:

function debounceUntilHidden(callback) {
    var bound = false, debounced = _.debounce(callback);

    function handleHidden() {
        if (document.visibilityState !== 'hidden') return;
        debounced.cancel();
        document.removeEventListener('visibilitychange', handleHidden, {
            capture: true,
        });
        bound = false;
        callback();
    }

    return function() {
        if (!bound) {
            document.addEventListener('visibilitychange', handleHidden, {
                capture: true,
                passive: true,
            });
            bound = true;
        }
        debounced();
    };
}

var handler = debounceUntilHidden(expensiveAPICall);
document.querySelector('#button').addEvenListener('click', handler);

This will give you a clean separation of concerns and responsibilities. Your debounceUntilHidden can do exactly what you need it to do, and you can reuse it everywhere. At the same time, you can rely on _.debounce to behave the same way it has always done.

I release the above example code under the unlicense; you are free to adjust and use it, without attribution, at your own risk. I hope this addresses your use case sufficiently.

I will now close this pull request, but you can still reply if you wish.