fast-memoize.js icon indicating copy to clipboard operation
fast-memoize.js copied to clipboard

Doesn't memoize the result for keys returning `undefined`

Open hargasinski opened this issue 6 years ago • 4 comments

When the memoized function returns undefined for a specific parameter, another call to the memoized function runs the original function again as opposed to fetching the result from the cache.

Reproducible Example:

const memoize = require('fast-memoize')
const assert = require('assert')

let numCalled = 0
function longProcess(arg) {
  ++numCalled
  return
}

// memoize result
const memoizedProcess = memoize(longProcess)
memoizedProcess("foo")

// get memoized result
memoizedProcess("foo")
assert(numCalled === 1)

It seems like this changed from v2.2.8 to v2.3.0. Is this the intended behaviour?

hargasinski avatar Mar 08 '18 05:03 hargasinski

I see how this could be the expected behaviour though, i.e. if the function returns undefined, it could mean something in the function failed and it should be run again, e.g. it run before a global object was initialised/some data wasn't fetched yet.

hargasinski avatar Mar 08 '18 05:03 hargasinski

@hargasinski have you benchmarked using a Map cache with .has() or .hasOwnProperty/in with the current null-prototype object cache?

qm3ster avatar Feb 25 '19 15:02 qm3ster

Hmmm, in my opinion this is an unexpected behaviour if not explicitly stated otherwise.

simonfan avatar Jul 01 '21 23:07 simonfan

I agree that it should be documented somewhere.

I checked the source code (line 36) and looks like it calls the original function whenever the cached result is undefined.

(Usually, undefined means something is not in the cache yet, so it calls the original function and caches this result. However, this means it no longer memoizes if the original function actually returns undefined).

https://github.com/caiogondim/fast-memoize.js/blob/master/src/index.js#L36

thawsitt avatar Oct 25 '21 21:10 thawsitt