Merge pull request #18314 from emberjs/bugfix/use-class-inheritance-f…

…or-getters-setters [BUGFIX beta] Use class inheritance for getters and setters
emberjs · Aug 28, 2019 · dce5264 · dce5264
2 parents 8b43936 + f046bbe
commit dce5264
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 32 deletions.
diff --git a/packages/@ember/-internals/metal/lib/computed.ts b/packages/@ember/-internals/metal/lib/computed.ts
@@ -630,6 +630,8 @@ export class ComputedProperty extends ComputedDescriptor {
       } finally {
         endPropertyChanges();
       }
+
+      return ret;
     } else {
       return this.setWithSuspend(obj, keyName, value);
     }

diff --git a/packages/@ember/-internals/metal/lib/decorator.ts b/packages/@ember/-internals/metal/lib/decorator.ts
@@ -1,6 +1,7 @@
 import { Meta, meta as metaFor } from '@ember/-internals/meta';
 import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
 import { assert } from '@ember/debug';
+import { _WeakSet as WeakSet } from '@ember/polyfills';
 import { setClassicDecorator } from './descriptor_map';
 import { unwatch, watch } from './watching';
 
@@ -140,11 +141,17 @@ function DESCRIPTOR_SETTER_FUNCTION(
   name: string,
   descriptor: ComputedDescriptor
 ): (value: any) => void {
-  return function CPSETTER_FUNCTION(this: object, value: any): void {
+  let func = function CPSETTER_FUNCTION(this: object, value: any): void {
     return descriptor.set(this, name, value);
   };
+
+  CP_SETTER_FUNCS.add(func);
+
+  return func;
 }
 
+export const CP_SETTER_FUNCS = new WeakSet();
+
 export function makeComputedDecorator(
   desc: ComputedDescriptor,
   DecoratorClass: { prototype: object }

diff --git a/packages/@ember/-internals/metal/lib/property_get.ts b/packages/@ember/-internals/metal/lib/property_get.ts
@@ -111,9 +111,11 @@ export function get(obj: object, keyName: string): any {
       }
     }
 
-    let descriptor = descriptorForProperty(obj, keyName);
-    if (descriptor !== undefined) {
-      return descriptor.get(obj, keyName);
+    if (!EMBER_METAL_TRACKED_PROPERTIES) {
+      let descriptor = descriptorForProperty(obj, keyName);
+      if (descriptor !== undefined) {
+        return descriptor.get(obj, keyName);
+      }
     }
 
     if (DEBUG && HAS_NATIVE_PROXY) {

diff --git a/packages/@ember/-internals/metal/lib/property_set.ts b/packages/@ember/-internals/metal/lib/property_set.ts
@@ -9,6 +9,7 @@ import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
 import { assert } from '@ember/debug';
 import EmberError from '@ember/error';
 import { DEBUG } from '@glimmer/env';
+import { CP_SETTER_FUNCS } from './decorator';
 import { descriptorForProperty } from './descriptor_map';
 import { isPath } from './path_cache';
 import { MandatorySetterFunction } from './properties';
@@ -85,11 +86,22 @@ export function set(obj: object, keyName: string, value: any, tolerant?: boolean
   }
 
   let meta = peekMeta(obj);
-  let descriptor = descriptorForProperty(obj, keyName, meta);
 
-  if (descriptor !== undefined) {
-    descriptor.set(obj, keyName, value);
-    return value;
+  if (!EMBER_METAL_TRACKED_PROPERTIES) {
+    let descriptor = descriptorForProperty(obj, keyName, meta);
+
+    if (descriptor !== undefined) {
+      descriptor.set(obj, keyName, value);
+      return value;
+    }
+  } else {
+    let descriptor = lookupDescriptor(obj, keyName);
+    let setter = descriptor === null ? undefined : descriptor.set;
+
+    if (setter !== undefined && CP_SETTER_FUNCS.has(setter)) {
+      obj[keyName] = value;
+      return value;
+    }
   }
 
   let currentValue: any;

diff --git a/packages/@ember/-internals/metal/tests/accessors/get_test.js b/packages/@ember/-internals/metal/tests/accessors/get_test.js
@@ -1,6 +1,6 @@
 import { ENV } from '@ember/-internals/environment';
 import { Object as EmberObject } from '@ember/-internals/runtime';
-import { get, getWithDefault, Mixin, observer } from '../..';
+import { get, getWithDefault, Mixin, observer, computed } from '../..';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 import { run } from '@ember/runloop';
 
@@ -313,5 +313,50 @@ moduleFor(
         'should return the set value, not false'
       );
     }
+
+    ['@test should respect prototypical inheritance when subclasses override CPs'](assert) {
+      let ParentClass = EmberObject.extend({
+        prop: computed({
+          get() {
+            assert.ok(false, 'incorrect getter called');
+            return 123;
+          },
+        }),
+      });
+
+      let SubClass = ParentClass.extend({
+        get prop() {
+          assert.ok(true, 'correct getter called');
+          return 456;
+        },
+      });
+
+      let instance = SubClass.create();
+
+      instance.prop;
+    }
+
+    ['@test should respect prototypical inheritance when subclasses override CPs with native classes'](
+      assert
+    ) {
+      class ParentClass extends EmberObject {
+        @computed
+        get prop() {
+          assert.ok(false, 'incorrect getter called');
+          return 123;
+        }
+      }
+
+      class SubClass extends ParentClass {
+        get prop() {
+          assert.ok(true, 'correct getter called');
+          return 456;
+        }
+      }
+
+      let instance = SubClass.create();
+
+      instance.prop;
+    }
   }
 );
diff --git a/packages/@ember/-internals/metal/tests/accessors/set_test.js b/packages/@ember/-internals/metal/tests/accessors/set_test.js
@@ -1,4 +1,5 @@
-import { get, set, trySet } from '../..';
+import { Object as EmberObject } from '@ember/-internals/runtime';
+import { get, set, trySet, computed } from '../..';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 moduleFor(
@@ -142,5 +143,50 @@ moduleFor(
       assert.equal(count, 1, 'should have native setter');
       assert.equal(get(obj, 'foo'), 'computed bar', 'should return new value');
     }
+
+    ['@test should respect prototypical inheritance when subclasses override CPs'](assert) {
+      let ParentClass = EmberObject.extend({
+        prop: computed({
+          set(key, val) {
+            assert.ok(false, 'incorrect setter called');
+            this._val = val;
+          },
+        }),
+      });
+
+      let SubClass = ParentClass.extend({
+        set prop(val) {
+          assert.ok(true, 'correct setter called');
+          this._val = val;
+        },
+      });
+
+      let instance = SubClass.create();
+
+      instance.prop = 123;
+    }
+
+    ['@test should respect prototypical inheritance when subclasses override CPs with native classes'](
+      assert
+    ) {
+      class ParentClass extends EmberObject {
+        @computed
+        set prop(val) {
+          assert.ok(false, 'incorrect setter called');
+          this._val = val;
+        }
+      }
+
+      class SubClass extends ParentClass {
+        set prop(val) {
+          assert.ok(true, 'correct setter called');
+          this._val = val;
+        }
+      }
+
+      let instance = SubClass.create();
+
+      instance.prop = 123;
+    }
   }
 );
diff --git a/packages/@ember/-internals/utils/lib/lookup-descriptor.ts b/packages/@ember/-internals/utils/lib/lookup-descriptor.ts
@@ -1,4 +1,4 @@
-export default function lookupDescriptor(obj: object, keyName: string) {
+export default function lookupDescriptor(obj: object, keyName: string | symbol) {
   let current: object | null = obj;
   do {
     let descriptor = Object.getOwnPropertyDescriptor(current, keyName);

diff --git a/packages/@ember/-internals/utils/lib/mandatory-setter.ts b/packages/@ember/-internals/utils/lib/mandatory-setter.ts
@@ -1,6 +1,7 @@
 import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
 import { assert } from '@ember/debug';
 import { DEBUG } from '@glimmer/env';
+import lookupDescriptor from './lookup-descriptor';
 
 export let setupMandatorySetter: ((obj: object, keyName: string | symbol) => void) | undefined;
 export let teardownMandatorySetter: ((obj: object, keyName: string | symbol) => void) | undefined;
@@ -17,31 +18,12 @@ if (DEBUG && EMBER_METAL_TRACKED_PROPERTIES) {
     { [key: string | symbol]: PropertyDescriptorWithMeta }
   > = new WeakMap();
 
-  let getPropertyDescriptor = function(
-    obj: object,
-    keyName: string | symbol
-  ): PropertyDescriptorWithMeta | undefined {
-    let current = obj;
-
-    while (current !== null) {
-      let desc = Object.getOwnPropertyDescriptor(current, keyName);
-
-      if (desc !== undefined) {
-        return desc;
-      }
-
-      current = Object.getPrototypeOf(current);
-    }
-
-    return;
-  };
-
   let propertyIsEnumerable = function(obj: object, key: string | symbol) {
     return Object.prototype.propertyIsEnumerable.call(obj, key);
   };
 
   setupMandatorySetter = function(obj: object, keyName: string | symbol) {
-    let desc = getPropertyDescriptor(obj, keyName) || {};
+    let desc = (lookupDescriptor(obj, keyName) as PropertyDescriptorWithMeta) || {};
 
     if (desc.get || desc.set) {
       // if it has a getter or setter, we can't install the mandatory setter.
@@ -113,7 +95,7 @@ if (DEBUG && EMBER_METAL_TRACKED_PROPERTIES) {
         // If the object didn't have own property before, it would have changed
         // the enumerability after setting the value the first time.
         if (!setter.hadOwnProperty) {
-          let desc = getPropertyDescriptor(obj, keyName);
+          let desc = lookupDescriptor(obj, keyName);
           desc!.enumerable = true;
 
           Object.defineProperty(obj, keyName, desc!);