Fix Issue #93: Prevent Getters from Being Called During Decoration and Preserve this Context

[Smoke3785]

Description:

Howdy everyone,

This PR addresses issue #93, where getters were being invoked during the decoration phase, leading to unintended side effects such as ReferenceError due to premature access before class initialization. Previously, attempts by @neodon to resolve this issue were effective in some cases but failed to preserve correct this context.

Proposed Solution:

To circumvent the loss of this context when reassigning getter implementation, I simply wrapped the original getter in a non-memoized function. This wrapped function is then assigned as the new getter, ensuring that the getter logic executes only when accessed without interfering with class instantiation or dependencies.

Key Points:

1. Method Memoization:

• Each instance method decorated with @memoizeDecorator now has its own memoized function stored using a unique Symbol so that memoization caches are per-instance.

2. Getter Handling:

• Currently, getters are wrapped in a function that caches their returned value using a unique Symbol. However, this leads to stale data if the underlying properties change after the first access. This is how memoization is meant to work, but it isn't completely intuitive.
• It might be worth adding a parameter to the options that accepts a callback that, as a side effect, clears the cache for the given method. I haven't tested passing in a reference to the method on the prototype, perhaps that works as well.

3. Weakmap Usage

• I didn't try to replicate your weakmap strategy to store the cache. If you'd like to replace the use of symbol with a weakmap, by all means.

Testing

The current implementation passes all existing tests - it may be worth adding some to ensure this is preserved properly. I'm not sure what specific issues @neodon ran into preserving this, but the following code behaves as expected:

class Example {
  index = 0;

  @memoizeDecorator()
  counter(s: string) {
    return ++this.index;
  }

  @memoizeDecorator()
  get count() {
    return this.index + 1;
  }
}

const test = new Example();

console.log(test.count); // Expected: 1
test.index = 1;
console.log(test.count); // Expected: 2

console.log(test.counter("a")); // Expected: 2
console.log(test.counter("a")); // Expected: 2 (cached)
console.log(test.counter("b")); // Expected: 3
console.log(test.counter("c")); // Expected: 4
console.log(test.counter("a")); // Expected: 2 (cached)

Hope this helps!
Owen Rossi-Keen, Iliad.dev

[sindresorhus]

Thanks for working on this

[sindresorhus]

It should use WeakMap, not Symbol.

[Smoke3785]

I'll implement with weakmap and resubmit. Thanks for taking a look!

[Smoke3785]

Hey Sindre. The cache should be stored on each instance in a WeakMap. It passes tests and seems to work as far as I'm able to test, but I am not confident my implementation plays well with the cache invalidation / key calculation options.

Do you have any suggestions for methodDecorator-specific test-cases to try?

I hope this addresses your concerns. Thank you again for reviewing,
Owen Rossi-Keen, Iliad.dev