Skip to content

Commit

Permalink
avoid double define for native descriptors
Browse files Browse the repository at this point in the history
  • Loading branch information
bekzod committed Sep 17, 2018
1 parent 93f4d26 commit 246435e
Show file tree
Hide file tree
Showing 8 changed files with 27 additions and 39 deletions.
1 change: 1 addition & 0 deletions packages/@ember/-internals/metal/lib/alias.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export class AliasedProperty extends Descriptor implements DescriptorWithDepende

setup(obj: object, keyName: string): void {
assert(`Setting alias '${keyName}' on self`, this.altKey !== keyName);
super.setup(obj, keyName);
let meta = metaFor(obj);
if (meta.peekWatching(keyName) > 0) {
addDependentKeys(this, obj, keyName, meta);
Expand Down
10 changes: 5 additions & 5 deletions packages/@ember/-internals/metal/lib/descriptor.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
import { Descriptor as EmberDescriptor } from './properties';
import { Descriptor } from './properties';

export default function descriptor(desc: PropertyDescriptor) {
return new Descriptor(desc);
return new NativeDescriptor(desc);
}

/**
A wrapper for a native ES5 descriptor. In an ideal world, we wouldn't need
this at all, however, the way we currently flatten/merge our mixins require
a special value to denote a descriptor.
@class Descriptor
@class NativeDescriptor
@private
*/
class Descriptor extends EmberDescriptor {
class NativeDescriptor extends Descriptor {
desc: PropertyDescriptor;
enumerable: boolean;

constructor(desc: PropertyDescriptor) {
super();
this.desc = desc;
this.enumerable = desc.enumerable !== false;
this.configurable = desc.configurable !== false;
}

setup(obj: object, key: string) {
Expand Down
26 changes: 11 additions & 15 deletions packages/@ember/-internals/metal/lib/properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,18 @@ export type InheritingGetterFunction = ((this: object) => void) & {
@private
*/
export abstract class Descriptor {
isDescriptor: boolean;
enumerable: boolean;
constructor() {
this.isDescriptor = true;
this.enumerable = true;
isDescriptor = true;
enumerable = true;
configurable = true;

setup(obj: object, keyName: string): void {
Object.defineProperty(obj, keyName, {
enumerable: this.enumerable,
configurable: this.configurable,
get: DESCRIPTOR_GETTER_FUNCTION(keyName, this),
});
}

setup(_obj: object, _keyName: string): void {}
teardown(_obj: object, _keyName: string, _meta: Meta): void {}

abstract get(obj: object, keyName: string): any | null | undefined;
Expand Down Expand Up @@ -188,16 +192,8 @@ export function defineProperty(
let value;
if (desc instanceof Descriptor) {
value = desc;

Object.defineProperty(obj, keyName, {
configurable: true,
enumerable,
get: DESCRIPTOR_GETTER_FUNCTION(keyName, value),
});

meta.writeDescriptors(keyName, value);

desc.setup(obj, keyName);
meta.writeDescriptors(keyName, value);
} else if (desc === undefined || desc === null) {
value = data;

Expand Down
8 changes: 0 additions & 8 deletions packages/@ember/-internals/metal/lib/property_get.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,6 @@ export function get(obj: object, keyName: string): any {
}
);

Object.defineProperty(obj, keyName, {
configurable: true,
enumerable: value.enumerable === false,
get() {
return value.get(this, keyName);
},
});

meta(obj).writeDescriptors(keyName, value);

value.setup(obj, keyName);
Expand Down
8 changes: 0 additions & 8 deletions packages/@ember/-internals/metal/lib/property_set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,6 @@ export function set(obj: object, keyName: string, value: any, tolerant?: boolean

let cv: Descriptor = currentValue;

Object.defineProperty(obj, keyName, {
configurable: true,
enumerable: cv.enumerable === false,
get() {
return cv.get(this, keyName);
},
});

meta(obj).writeDescriptors(keyName, cv);

cv.setup(obj, keyName);
Expand Down
10 changes: 8 additions & 2 deletions packages/@ember/-internals/metal/tests/descriptor_test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Object as EmberObject } from '@ember/-internals/runtime';
import { Mixin, defineProperty, descriptor } from '..';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
import { isEdge } from 'internal-test-helpers';

let classes = [
class {
Expand Down Expand Up @@ -148,7 +149,7 @@ let classes = [
},
];

classes.forEach(TestClass => {
classes.forEach((TestClass, testCaseIndex) => {
moduleFor(
TestClass.module('@ember/-internals/metal/descriptor'),
class extends AbstractTestCase {
Expand Down Expand Up @@ -182,7 +183,12 @@ classes.forEach(TestClass => {

let source = factory.source();

assert.throws(() => delete source.foo, TypeError);
if (isEdge && testCaseIndex === 0) {
// https://github.com/emberjs/ember.js/pull/16741#issuecomment-420963181
assert.equal(delete source.foo, false);
} else {
assert.throws(() => delete source.foo, TypeError);
}

assert.throws(
() =>
Expand Down
2 changes: 1 addition & 1 deletion packages/internal-test-helpers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@ export {
ModuleBasedResolver as ModuleBasedTestResolver,
} from './lib/test-resolver';

export { isIE11 } from './lib/browser-detect';
export { isIE11, isEdge } from './lib/browser-detect';
export { verifyInjection, verifyRegistration } from './lib/registry-check';
1 change: 1 addition & 0 deletions packages/internal-test-helpers/lib/browser-detect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ declare global {
}
}
export const isIE11 = !window.ActiveXObject && 'ActiveXObject' in window;
export const isEdge = /Edge/.test(navigator.userAgent);

0 comments on commit 246435e

Please sign in to comment.