erikras / lru-memoize

A utility to provide LRU memoization for any js function
MIT License
316 stars 20 forks source link

deepEquals behavior #78

Open dpwrussell opened 8 years ago

dpwrussell commented 8 years ago

I'm a bit confused by the deepEquals behaviour here. As there is an option to enable deepObject equivalence I was not expecting objects to be equivalent (with deepObject=false) in this scenario.

const memoize = require('lru-memoize');

let multiply = (a, b, c) => {
  console.log('Multiplying', a, b, c);
  return a.val * b.val * c.val;
};

multiply = memoize(10)(multiply);

const a = {val:1};
const b = {val:2};
const c = {val:3};

const a2 = {val:1};

multiply(a,b,c)
multiply(a,b,c)
multiply(a2,b,c)

// Output
// Multiplying { val: 1 } { val: 2 } { val: 3 }

I was expecting that the multiple calculation would be run twice. Once for the first multiply(a,b,c) and again for the multiply(a2,b,c). The same is true for arrays.

I dug into the code a little and it looks like the intention is to penetrate 1 level deep by default?

As far as I know the arguments to a function will always be the array-like Arguments object, and I don't think that would ever be the same across function calls so it would make sense to me to start with something that handles the array specifically and then drops into the shallow or deepEquals possibilities.

Perhaps deepEquals and deepObject equals should be separate arguments, but I've reused deepObject in this example PR: https://github.com/erikras/lru-memoize/pull/77

It should be easy to see the current behaviour for these unit tests by putting:

const cache = createCache(limit, deepEquals(equals, deepObjects));

back in.

benjie commented 7 years ago

To add to this: nested arrays are evaluated deeply even if deepObjects is false (perhaps that's why it's called deepObjects?)