bjoerge / debounce-promise

Create a debounced version of a promise returning function
MIT License
241 stars 27 forks source link

Fix/expensive promise #8

Closed vhiribarren closed 7 years ago

vhiribarren commented 7 years ago

Hello,

I have some problems when using the library with expensive Promises (they each take around 2 seconds).

If the cold debounced function is called with a second Promise is called and the first one is not finished, they share the same defer object. Here is a solution that seems to work for me, with a new test showing my problem.

If I actually do not correctly use the library and you think there is no bug, do not hesitate to tell me so.

Thank you!

bjoerge commented 7 years ago

Hi @vhiribarren! Thanks a lot for this. It made me reconsider how debounce-promise should work wrt. leading, and how it should deal with slow promises.

Currently, leading: true makes the subsequent calls that happens while the first promise is in flight, being resolved to the eventual value of the first promise, no matter what. I think this is the wrong/unexpected behavior, and what you instead want is the first call being invoked immediately and separately from the subsequent ones, regardless of how long the returned promise takes to resolve. IMO the real bug here is that the resolution time of each promise is even considered at all.

Here's an illustration of how I think it should be. (P(x) being the returned promise from each call):

function refresh() {
  return fetch('/my/api/something')
}

leading: false

const debounced = debounce(refresh, 100)
time (ms) ->   0 ---  10  ---  50  ---  100 ---
-----------------------------------------------
debounced()    | --- P(1) --- P(1) --- P(1) ---
refresh()      | --------------------- P(1) ---

leading: true

const debounced = debounce(refresh, 100, {leading: true})
time (ms) ->   0 ---  10  ---  50  ---  100 ---  110 ---
--------------------------------------------------------
debounced()    | --- P(1) --- P(2) --- P(2) --- P(2) ---
refresh()      | --- P(1) --------------------- P(2) ---

If I'm reading you correctly, what you want to achieve is something that could be built on top of debounce-promise pretty easily by a function that makes sure theres only one promise in flight simultaneously, e.g.:

function sequentialize (fn) {
  let current = null
  return function serial (...args) {
    if (!current) {
      current = fn(...args).then(val => {
        current = null
        return val
      })
    }
    return current
  }
}
// Usage:
const mydebounced = sequentialize(debounce(myFunction, 300, {leading: true}))

Let me know what you think.

bjoerge commented 7 years ago

The behavior I described above is published in v3.0. Thanks!

vhiribarren commented 7 years ago

Hello,

my problem was with leading=false

function sleep (id) {
  return new Promise(resolve => setTimeout(resolve, 200))
}

const debounced = debounce(sleep, 20)

time (ms) ->       0 --  10  --  20  -- 40 --  100 --  110 -- 130 --- 240 -- 330 -
----------------------------------------------------------------------------------
debounced()        | -- P(1) -- P(2) --------- P(3) -- P(4) ----------------------
sleep()            | ----------------  P(2) ----------------- P(4) ---------------
Promise resolution | ------------------------------------------------ P(2) -- P(2)
                                                                               |
                 P(4) resolve with the same defer object than P(2) ------------|

The test I added was showing that. Anyway, your new version 3.0.x corrects the problem I had.

About your rework with leading=true, I completely understand your opinion and I agree with it in most part. However, I find the result confusing.

Your example is:

var debounce = require('debounce-promise')

function expensiveOperation(value) {
  return Promise.resolve(value)
}

var saveCycles = debounce(expensiveOperation, 100, {leading: true});

[1, 2, 3, 4].forEach(num => {
  return saveCycles('call no #' + num).then(value => {
    console.log(value)
  })
})

//=> call no #1
//=> call no #4
//=> call no #4
//=> call no #4

The fact that no #4 is then called seems like the algorithm switched to leading=false for the second part of the list, for me (as if leading=true for [1] and then leading=false for [2, 3, 4].