From f046bbe18c0f476a0bb3467d6a2cae2f0588d90c Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Fri, 23 Aug 2019 10:49:43 -0700 Subject: [PATCH] [BUGFIX beta] Use class inheritance for getters and setters Currently, `get` and `set` will bypass class inheritance and lookup descriptors for CPs. If they exist, they call them directly. This results in bugs where plain, undecorated getters and setters cannot override CPs on parent classes. This fix converts us to using class inheritance, looking up descriptors recursively on the class to find the actual property that is meant to be accessed for this instance. In the `get` case, this is simple removing the bypass altogether. For the `set` case, this would result in behavioral differences: * An additional `get` triggered just before setting, since `set` gets the previous value in order to know if it should notify. * An additional property change notification for CPs, since they must notify themselves (when set using a native setter, for instance). Instead, this PR looks up the descriptor and checks to see if it's a `CPSETTER` function, and triggers it directly if so. --- .../@ember/-internals/metal/lib/computed.ts | 2 + .../@ember/-internals/metal/lib/decorator.ts | 9 +++- .../-internals/metal/lib/property_get.ts | 8 ++-- .../-internals/metal/lib/property_set.ts | 20 ++++++-- .../metal/tests/accessors/get_test.js | 47 +++++++++++++++++- .../metal/tests/accessors/set_test.js | 48 ++++++++++++++++++- .../-internals/utils/lib/lookup-descriptor.ts | 2 +- .../-internals/utils/lib/mandatory-setter.ts | 24 ++-------- 8 files changed, 128 insertions(+), 32 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/computed.ts b/packages/@ember/-internals/metal/lib/computed.ts index 08405fd2c81..3d7a14cd9e6 100644 --- 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 index a130691606f..9f7bafd17aa 100644 --- 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 index e0b6f37ef0b..0251d4f4670 100644 --- 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 index 2b21a846296..909128a7a2b 100644 --- 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 index f121499e91d..b3ce67788b3 100644 --- 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 index d10006cd246..4b80cbef6ff 100644 --- 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 index c450b34fa49..395dea6ebde 100644 --- 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 index b9ce32bdc64..7b856eedb73 100644 --- 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!);