Non-primitive function parameters

[agudulin]

The current implementation of memoize doesn't work correctly with non-primitive function parameters.

E.g. look at this test:

it('should not overwrite non-primitive arguments', next => {
  const cache = Object.create(null)
  let c = memoize((key => key), { cache })

  const key1 = { a: 1 }
  const key2 = { a: 2 }
  c(key1)
  c(key2)

  expect(Object.keys(cache)).to.have.length(2)
  // ... cache is { '[Object object]': { a: 2 } } now

  next()
});

Do you think it would make sense to print a corresponding error message when they try to pass any non-primitive ref as a parameter?

WeakMap could be a solution for this problem (for memory leak also).

cc @stryju

[developit]

Only primitive keys are supported, and I believe only the first argument to the function is used as a key. I'm open to exploring more correct memoization, but the cost of switching to WeakMap would probably kill any benefit for the majority case.

FWIW, I don't think decko should try to implement a perfect memoization - for that I would recommend fast-memoize. I envisioned Decko's memoize are more for basic tasks like 0-1 arity functions, particularly those that return Promises - that's where memoization benefit is maximized.

[agudulin]

So if you want to keep only primitives, would this small check be enough? :)

+ const isPrimitive = require('is-primitive')
// ...
memoize(fn, opt=EMPTY) {
    let cache = opt.cache || {};
    return function(...a) {
        let k = String(a[0]);

+               if (!isPrimitive(k)) throw new Error('meow meow watch out!')

        if (opt.caseSensitive===false) k = k.toLowerCase();
        return HOP.call(cache,k) ? cache[k] : (cache[k] = fn.apply(this, a));
    };
}
// ...

[developit]

Depends how big is-primitive is 😋

[agudulin]

Not that big (1LOC function): https://github.com/jonschlinkert/is-primitive/blob/master/index.js#L11-L13

const isPrimitive = value =>
  value == null || (typeof value !== 'function' && typeof value !== 'object')

[developit]

Not too shabby yeah. I guess I'm just torn on whether this falls to a documentation issue, a missing runtime exception (as you suggested), or trying to come up with some simple way to support complex objects. Hard to pinpoint since the lib is fairly generic.

[stryju]

to add to this, here's a list of false-positive cache hits:

m(1) === m('1')
m(NaN) === m('NaN')
m([]) === m([,]) === m('')
m(['1,2', 3]) === m([1, 2, 3]) === m('1,2,3') // and so on
m(false) === m('false')
m(undefined) === m('undefined') === m(window.all) // I know, right?!..
// ... and so on

It would be "OK" assuming this would be used for functions with single-type param

---

on a different note, memoize() will not work properly for functions with more than one param

[developit]

yup, all correct and all currently true. Problem is I've used this lib quite a bit for things under the assumption only the first argument is the cache key and others are ignored. One such example would be caching GET requests through memoize(fetch) - passing any options object would result in a failing reference equality check for the second argument.

Even with a WeakMap-implemented key setup, the following would be awkward:

m(['a', 'b']) !== m(['a', 'b'])  // seems like it ought to be true, but the arrays aren't referentially equal