andreypopp/memoize-decorator

Memoize getters and methods to compute only once

memoize-decorator's Issues

Make stored value non-enumerable.

The memoized value is currently stored in an enumerable property (_memoized_xxx), but this messes up deep equality comparisons. The solutions to this would either be:

• Keep the same property name but set it using Object.defineProperty with enumerable set to false.
• Use symbols instead of a magic property name.
• Use WeakMaps, similar to the above.

What do you think?

Does a memoized function return a new value if called with different arguments?

If not, could this behaviour be added?

Doesn't work with modern babel 7

Hi! Tried to use your package, got the following error:

/home/mi/lenin/news-parser/node_modules/memoize-decorator/lib/index.js:16
  if (typeof descriptor.value === 'function') {
                        ^

TypeError: Cannot read property 'value' of undefined
    at memoize (/home/mi/lenin/news-parser/node_modules/memoize-decorator/lib/index.js:16:25)
    at Object.decorateElement (/home/mi/lenin/news-parser/tests/keka.js:9:2704)
    at Object.<anonymous> (/home/mi/lenin/news-parser/tests/keka.js:9:1594)
    at Array.forEach (<anonymous>)
    at Object.decorateClass (/home/mi/lenin/news-parser/tests/keka.js:9:1467)
    at _decorate (/home/mi/lenin/news-parser/tests/keka.js:7:308)
    at Object.<anonymous> (/home/mi/lenin/news-parser/tests/keka.js:35:9)
    at Module._compile (internal/modules/cjs/loader.js:816:30)
    at Module._compile (/home/mi/lenin/news-parser/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:827:10)

Code:

import memoize from 'memoize-decorator'

class A {
  @memoize
  test() {
    console.log('heavy computation')
    return 42
  }
}

let a = new A()
a.test() // prints 'heavy computations', returns 42
a.test() // just returns 42

.babelrc

{
  "presets": [
    [
      "@babel/preset-env",
      {
        "targets": {
          "node": "current"
        }
      }
    ]
  ],
  "plugins": [
    "transform-numeric-separator",
    ["@babel/plugin-proposal-decorators", { "decoratorsBeforeExport": true }],
    "@babel/plugin-proposal-class-properties",
    "@babel/plugin-proposal-optional-chaining",
    "@babel/plugin-proposal-export-default-from",
    "@babel/plugin-proposal-export-namespace-from"
  ]
}

How I ran it:

node -r @babel/register test.js

or via

babel-node test.js

---

More debugging:

function memoize(target, name, descriptor) {
  console.log('target:', target)
  console.log('name:', name)
  console.log('descriptor:', descriptor)
  ...
}

Result:

target: Object [Descriptor] {
  kind: 'method',
  key: 'test',
  placement: 'prototype',
  descriptor:
   { value: [Function: test],
     writable: true,
     configurable: true,
     enumerable: false } }
name: undefined
descriptor: undefined
/home/mi/lenin/news-parser/node_modules/memoize-decorator/lib/index.js:19
  if (typeof descriptor.value === 'function') {
                        ^

Memoize-decorator doesn't work with frozen objects

I'm wondering if you'd be open to using a Map instead, if its available (and fall back to the existing implementation). Basically, instead of setting this[memoizedName] (which breaks after Object.freeze(this)), you could have a nested set of maps, of the type:

outerMap: Map<instance, innerMap>
innerMap: Map<getter, value | SENTINEL>

FWIW, I'm not 100% sure that we need a set of nested maps. Each getter could be a different object on every instance, but I'm assuming that JS reuses the same object.

Happy to make a PR with that.