Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Use class inheritance for getters and setters #18314

Merged
merged 1 commit into from
Aug 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/@ember/-internals/metal/lib/computed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,8 @@ export class ComputedProperty extends ComputedDescriptor {
} finally {
endPropertyChanges();
}

return ret;
} else {
return this.setWithSuspend(obj, keyName, value);
}
Expand Down
9 changes: 8 additions & 1 deletion packages/@ember/-internals/metal/lib/decorator.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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 }
Expand Down
8 changes: 5 additions & 3 deletions packages/@ember/-internals/metal/lib/property_get.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
20 changes: 16 additions & 4 deletions packages/@ember/-internals/metal/lib/property_set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I don't think we have a way to test the path where EMBER_METAL_TRACKED_PROPERTIES is disabled (since its on by default now). If we end up having to pull it from 3.13, we'll probably have to manually debug some of these scenarios.

Not really a big deal (we don't expect to pull it), but just wanted to mention it so others are aware...

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;
Expand Down
47 changes: 46 additions & 1 deletion packages/@ember/-internals/metal/tests/accessors/get_test.js
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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;
}
}
);
48 changes: 47 additions & 1 deletion packages/@ember/-internals/metal/tests/accessors/set_test.js
Original file line number Diff line number Diff line change
@@ -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(
Expand Down Expand Up @@ -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;
}
}
);
2 changes: 1 addition & 1 deletion packages/@ember/-internals/utils/lib/lookup-descriptor.ts
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
24 changes: 3 additions & 21 deletions packages/@ember/-internals/utils/lib/mandatory-setter.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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!);
Expand Down