Repository: exadel-inc/esl
Pull request #1176 (2022-08-26 00:57:49)

fix(esl-utils): `@decorate` and `@bind` complete rework


Patch:
diff --git a/src/modules/esl-utils/decorators/bind.ts b/src/modules/esl-utils/decorators/bind.ts
index 546055af4..e59211a42 100644
--- a/src/modules/esl-utils/decorators/bind.ts
+++ b/src/modules/esl-utils/decorators/bind.ts
@@ -1,47 +1,33 @@
-const BINDINGS_STORE_KEY = '__fnBindings__';
+import {getPropertyDescriptor} from '../misc/object/utils';
+
 /** Decorator "bind" allows to bind prototype method context to class instance */
 // eslint-disable-next-line @typescript-eslint/ban-types
-export function bind<T extends Function>(target: object,
-                                         propertyKey: string,
-                                         descriptor: TypedPropertyDescriptor<T>): TypedPropertyDescriptor<T> {
+export function bind<Fn extends Function>(target: object,
+                                          propertyKey: string,
+                                          descriptor: TypedPropertyDescriptor<Fn>): TypedPropertyDescriptor<Fn> {
   // Validation check
   if (!descriptor || (typeof descriptor.value !== 'function')) {
     throw new TypeError('Only class methods can be decorated via @bind');
   }
   // Original function
-  const fn = descriptor.value;
+  const originalFn = descriptor.value;
 
   return {
     enumerable: descriptor.enumerable,
-    configurable: descriptor.configurable,
-
-    get(): T {
-      // Accessing via prototype returns original function
-      // If the constructor property is in the context then it's not an instance
-      if (!this || this === target || Object.hasOwnProperty.call(this, 'constructor')) {
-        return fn;
-      }
-
-      // Bounded functions store
-      let bindings = this[BINDINGS_STORE_KEY];
-      if (!bindings) {
-        bindings = this[BINDINGS_STORE_KEY] = new WeakMap<T, T>();
-      }
-
-      // Store binding if it does not exist
-      if (!bindings.has(fn)) {
-        bindings.set(fn, fn.bind(this));
-      }
+    configurable: true,
 
-      // Return binding
-      return bindings.get(fn);
+    get: function getBound(): Fn {
+      const proto = Object.getPrototypeOf(this);
+      const desc = getPropertyDescriptor(proto, propertyKey);
+      const isProtoCall = !desc || desc.get !== getBound;
+      return isProtoCall ? originalFn : (this[propertyKey] = originalFn.bind(this));
     },
-    set(value: T): void {
+    set(value: Fn): void {
       Object.defineProperty(this, propertyKey, {
-        writable: true,
-        enumerable: false,
-        configurable: true,
-        value
+        value,
+        writable: descriptor.writable,
+        enumerable: descriptor.enumerable,
+        configurable: true
       });
     }
   };
diff --git a/src/modules/esl-utils/decorators/decorate.ts b/src/modules/esl-utils/decorators/decorate.ts
index 534893a40..a2b029170 100644
--- a/src/modules/esl-utils/decorators/decorate.ts
+++ b/src/modules/esl-utils/decorators/decorate.ts
@@ -1,3 +1,4 @@
+import {getPropertyDescriptor} from '../misc/object/utils';
 import type {AnyToAnyFnSignature} from '../misc/functions';
 
 /**
@@ -11,10 +12,35 @@ export function decorate<Args extends any[], Fn extends AnyToAnyFnSignature>(
   decorator: (fn: Fn, ...params: Args) => Fn,
   ...args: Args
 ) {
-  return function (target: any, propertyKey: string, descriptor: TypedPropertyDescriptor<Fn>): void {
+  return function (target: any, propertyKey: string, descriptor: TypedPropertyDescriptor<Fn>): TypedPropertyDescriptor<Fn> {
     if (!descriptor || typeof descriptor.value !== 'function') {
       throw new TypeError('Only class methods can be decorated');
     }
-    descriptor.value = decorator(descriptor.value, ...args);
+    const originalFn: Fn = descriptor.value;
+
+    return {
+      enumerable: descriptor.enumerable,
+      configurable: true,
+
+      get: function getBound(): Fn {
+        // Skip own properties
+        const proto = Object.getPrototypeOf(this);
+        // Find the closest descriptor for property
+        const desc = getPropertyDescriptor(proto, propertyKey);
+        // Return original function in case of prototype or super call
+        if (!desc || desc.get !== getBound) return originalFn;
+        const decoratedFn = decorator(originalFn.bind(this), ...args);
+        Object.setPrototypeOf(decoratedFn, originalFn);
+        return this[propertyKey] = decoratedFn;
+      },
+      set(value: Fn): void {
+        Object.defineProperty(this, propertyKey, {
+          value,
+          writable: descriptor.writable,
+          enumerable: descriptor.enumerable,
+          configurable: true
+        });
+      }
+    };
   };
 }
diff --git a/src/modules/esl-utils/decorators/listen.ts b/src/modules/esl-utils/decorators/listen.ts
index 4729c96be..2c03bf4c2 100644
--- a/src/modules/esl-utils/decorators/listen.ts
+++ b/src/modules/esl-utils/decorators/listen.ts
@@ -36,8 +36,7 @@ export function listen(desc: string | ESLListenerDescriptorExt): ListenDecorator
     if (typeof fn !== 'function') {
       throw new TypeError('Only class methods can be decorated via listener decorator');
     }
-
-    if (isDescriptorFn(descriptor.value) && descriptor.value.event !== desc.event) {
+    if (isDescriptorFn(fn) && fn.event !== desc.event) {
       throw new TypeError(`Method ${propertyKey} already decorated as ESLListenerDescriptor`);
     }
 


Test patch:
diff --git a/src/modules/esl-utils/decorators/test/bind.test.ts b/src/modules/esl-utils/decorators/test/bind.test.ts
index 60a5c3dcf..17a0f1a9c 100644
--- a/src/modules/esl-utils/decorators/test/bind.test.ts
+++ b/src/modules/esl-utils/decorators/test/bind.test.ts
@@ -1,73 +1,164 @@
 import {bind} from '../bind';
 
-describe('common @bind decorator test', () => {
-  test('basic binding test', () => {
-    const test = new TestClass('1');
-    const test2 = new TestClass('2');
-
-    expect(test.getThis.call(null)).toBe(test);
-    expect(test.getThis.call(null)).toBe(test);
-    expect(test2.getThis.call(null)).toBe(test2);
-
-    expect(test.getThis.call(null)).toBe(test);
-    expect(test.getThis).toBe(test.getThis);
-
-    expect(test.getThis).not.toBe(test2.getThis);
+describe('Common @bind decorator test', () => {
+  describe('Basic binding tests', () => {
+    class Test {
+      @bind
+      getThis() {
+        return this;
+      }
+    }
+    const instance1 = new Test();
+    const instance2 = new Test();
+
+    test(
+      'Decorated method accessed from instance has bound context',
+      () => expect(instance1.getThis.call(null)).toBe(instance1)
+    );
+    test(
+      'Decorated method second access from instance has bound context',
+      () => expect(instance1.getThis.call(null)).toBe(instance1)
+    );
+    test(
+      'Decorated method from another instance bound correctly',
+      () => expect(instance2.getThis.call(null)).toBe(instance2)
+    );
+
+    test(
+      'Bounded methods accessing uses cache to return the same function instance',
+      () => expect(instance1.getThis).toBe(instance1.getThis)
+    );
+    test(
+      'Bounded methods accessing should not return bounded function of another instance',
+      () => expect(instance1.getThis).not.toBe(instance2.getThis)
+    );
   });
 
-  test('basic context test', () => {
-    const test = new TestClass('1');
-    const test2 = new TestClass('2');
+  describe('Basic context test', () => {
+    class Test {
+      constructor(public name: string) {}
+      @bind
+      getThisName(): any {
+        return this.name;
+      }
+    }
 
-    expect(test.getThisName.call(null)).toBe('1');
-    expect(test2.getThisName.call(null)).toBe('2');
-    const cb = test2.getThisName;
-    expect(cb()).toBe('2');
+    test(
+      'Context of bounded method saved if method called outside ',
+      () => {
+        const instance = new Test('1');
+        const cb = instance.getThisName;
+        expect(cb()).toBe('1');
+      }
+    );
   });
 
-  test('inheritance basic binding test', () => {
-    class TestChildClass extends TestClass {
+  describe('Inheritance basic binding test', () => {
+    class TestParent {
+      @bind getThis() {
+        return this;
+      }
+    }
+    class TestChild extends TestParent {
     }
 
-    const test = new TestChildClass('3');
-    expect(test.getThis.call(null)).toBe(test);
-    expect(test.getThis.call(null)).toBe(test);
-    expect(test.getThis).toBe(test.getThis);
+    const instance = new TestChild();
+
+    test(
+      'Instance of inherited class has bounded method',
+      () => expect(instance.getThis.call(null)).toBe(instance)
+    );
+    test(
+      'Inherited bounded method saves context on the second call',
+      () => expect(instance.getThis.call(null)).toBe(instance)
+    );
+    test(
+      'Accessing bound inherited method returns the same method instance',
+      () => expect(instance.getThis).toBe(instance.getThis)
+    );
   });
 
-  test('inheritance prototype binding test', () => {
-    const test = new TestClass('1');
-    const test2 = new TestClass('2');
-    expect(TestClass.prototype.getThis.call(test)).toBe(test);
-    expect(TestClass.prototype.getThis.call(test2)).toBe(test2);
+  describe('Prototype access should return original method', () => {
+    class Test {
+      @bind getThis() {
+        return this;
+      }
+    }
+    const instance1 = new Test();
+    test(
+      'Prototype method direct call uses undecorated method',
+      () => expect(Test.prototype.getThis.call(instance1)).toBe(instance1)
+    );
+
+    const instance2 = new Test();
+    test(
+      'Attempt to call prototype method second time still respects the context',
+      () => expect(Test.prototype.getThis.call(instance2)).toBe(instance2)
+    );
   });
 
-  test('inheritance override binding test', () => {
-    const test = new TestClass('1');
-    const test2 = new TestClass('2');
-    expect(test.getThis.call(null)).toBe(test);
-    test.getThis = () => test2;
-    expect(test.getThis.call(null)).toBe(test2);
+  test('Bound method can be overwritten', () => {
+    class Test {
+      @bind getThis() {
+        return this;
+      }
+    }
+    const instance1 = new Test();
+    const instance2 = new Test();
+    expect(instance1.getThis.call(null)).toBe(instance1);
+    instance1.getThis = () => instance2;
+    expect(instance1.getThis.call(null)).toBe(instance2);
   });
 
-  test('inheritance super binding test', () => {
-    class TestChildClass extends TestClass {
-      @bind
-      getThisName() {
-        return '!' + super.getThisName();
+  describe('Inheritance super keyword should return original method', () => {
+    class TestParent {
+      public postfix: string;
+      @bind getThisName(): any {
+        return 'Hello' + (this.postfix || '');
+      }
+    }
+    class TestChild extends TestParent {
+      @bind getThisName() {
+        return super.getThisName() + ' world';
       }
       getThisSuperName() {
         return super.getThisName();
       }
+      getThisSuperNameMethod() {
+        return super.getThisName;
+      }
     }
 
-    const test = new TestChildClass('4');
-    expect(test.getThisName()).toBe('!4');
-    expect(test.getThisName.call(null)).toBe('!4');
-    expect(test.getThisSuperName()).toBe('4');
+    const instance = new TestChild();
+    test(
+      'Method override can access super method definition',
+      () => expect(instance.getThisName()).toBe('Hello world')
+    );
+    test(
+      'Overridden method accessing super method definition, itself bound correctly',
+      () => expect(instance.getThisName()).toBe('Hello world')
+    );
+    test(
+      'Other methods can access parent class method definition',
+      () => expect(instance.getThisSuperName()).toBe('Hello')
+    );
+
+    const instance2 = new TestChild();
+    instance2.postfix = '!';
+
+    test(
+      'Instances accessing superclass methods use correct context',
+      () => expect(instance2.getThisName()).toBe('Hello! world')
+    );
+
+    const instance3 = new TestChild();
+    test(
+      'Instances accessing superclass methods use exact unbounded version from prototype',
+      () => expect(instance3.getThisSuperNameMethod()).toBe(TestParent.prototype.getThisName)
+    );
   });
 
-  test('validation check', () => {
+  test('Decoration of illegal cass member throws error', () => {
     expect(() => {
       // eslint-disable-next-line @typescript-eslint/no-unused-vars
       class Test {
@@ -77,18 +168,3 @@ describe('common @bind decorator test', () => {
     }).toThrowError();
   });
 });
-
-class TestClass {
-  constructor(public name: string) {
-  }
-
-  @bind
-  getThis() {
-    return this;
-  }
-
-  @bind
-  getThisName(): any {
-    return this.name;
-  }
-}
diff --git a/src/modules/esl-utils/decorators/test/decorate.test.ts b/src/modules/esl-utils/decorators/test/decorate.test.ts
new file mode 100644
index 000000000..57c5b9f33
--- /dev/null
+++ b/src/modules/esl-utils/decorators/test/decorate.test.ts
@@ -0,0 +1,92 @@
+import {decorate} from '../decorate';
+
+describe('common @decorate decorator test', () => {
+  describe('Decorated method access leads to decorated instance access', () => {
+    const v1 = Symbol('v1');
+    const fn1 = jest.fn(() => v1);
+
+    const v2 = Symbol('v2');
+    const fn2 =  jest.fn(() => v2);
+
+    const fnPull = [fn2, fn1];
+    const wrap = jest.fn((fn) => fnPull.pop() || ((...args: any[]) => fn(...args)));
+
+    const original = Symbol('original');
+    const originalFn =  jest.fn(() => original);
+    class Test {
+      @decorate(wrap)
+      test(...args: any[]): symbol { return originalFn.call(this, ...args); }
+    }
+
+    const t1 = new Test();
+    test('@decorator is lazy', () => expect(wrap).toBeCalledTimes(0));
+    test('Decorated with @decorator method returns wrapped method', () => expect(t1.test()).toBe(v1));
+    test('@decorator called ones it\'s accessed', () => expect(wrap).toBeCalledTimes(1));
+    test('Decorated with @decorator method returns wrapped method every time', () => expect(t1.test).toBe(fn1));
+    test('@decorator does not redecorate method second time', () => expect(wrap).toBeCalledTimes(1));
+
+    const t2 = new Test();
+    test('@decorator doesn\'t have effect on instance creation', () => expect(wrap).toBeCalledTimes(1));
+    test('Second instance creates its own wrapped method', () => expect(t2.test()).toBe(v2));
+    test('Decorated with @decorator method returns wrapped method every time', () => expect(t2.test).toBe(fn2));
+
+    const t3 = new Test();
+    test('Original function passed as a first argument for wrapper', () => expect(t3.test()).toBe(original));
+    test('Arguments are passed to the wrapped function', () => {
+      const arg1 = Symbol('arg1');
+      const arg2 = Symbol('arg1');
+      expect(t3.test(arg1, arg2)).toBe(original);
+      expect(originalFn).lastCalledWith(arg1, arg2);
+    });
+  });
+
+  describe('Decorated method inheritance works correct', () => {
+    const original = Symbol('original');
+    const originalFn =  jest.fn(() => original);
+    const wrap = jest.fn((fn) => ((...args: any[]) => fn(...args)));
+
+    class Parent {
+      @decorate(wrap)
+      test(...args: any[]): symbol { return originalFn.call(this, ...args); }
+    }
+
+    class Child extends Parent {
+      test(...args: any[]): symbol {
+        return super.test(...args);
+      }
+
+      testSuper(): any {
+        return super.test;
+      }
+    }
+
+    const instance = new Child();
+    const arg1 = Symbol('arg1');
+    test(
+      'Overwritten method define correctly and accessing original method',
+      () => expect(instance.test(arg1)).toBe(original)
+    );
+    test(
+      'Arguments passed correctly',
+      () => expect(originalFn).lastCalledWith(arg1)
+    );
+    test(
+      'Second call still works correct',
+      () => expect(instance.test(arg1)).toBe(original)
+    );
+    test(
+      'Prototype method call run function without decoration',
+      () => expect(Parent.prototype.test()).toBe(original)
+    );
+    test(
+      'Decoration does not happens on super access',
+      () => expect(wrap).toBeCalledTimes(0)
+    );
+
+    const instance2 = new Child();
+    test(
+      'Call of super keyword returns original method',
+      () => expect(instance2.testSuper()).toBe(Parent.prototype.test)
+    );
+  });
+});