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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX beta] Allow accessors in mixins #17710

Merged
merged 1 commit into from Mar 8, 2019
Merged

[BUGFIX beta] Allow accessors in mixins #17710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 33 additions & 2 deletions packages/@ember/-internals/metal/lib/mixin.ts
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ import { Meta, meta as metaFor, peekMeta } from '@ember/-internals/meta';
import {
getListeners,
getObservers,
getOwnPropertyDescriptors,
guidFor,
makeArray,
NAME_KEY,
Expand All @@ -22,7 +23,7 @@ import {
ComputedPropertyGetter,
ComputedPropertySetter,
} from './computed';
import { makeComputedDecorator } from './decorator';
import { makeComputedDecorator, nativeDescDecorator } from './decorator';
import {
descriptorForDecorator,
descriptorForProperty,
Expand Down Expand Up @@ -50,6 +51,36 @@ function isMethod(obj: any): boolean {
);
}

function isAccessor(desc: PropertyDescriptor) {
return typeof desc.get === 'function' || typeof desc.set === 'function';
}

function extractAccessors(properties: { [key: string]: any } | undefined) {
if (properties !== undefined) {
let descriptors = getOwnPropertyDescriptors(properties);
let keys = Object.keys(descriptors);
let hasAccessors = keys.some(key => isAccessor(descriptors[key]));
Copy link
Member

Choose a reason for hiding this comment

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

  1. I don't think we can rely on .some
  2. I'd prefer to avoid checking the same thing for isAccessor twice

Suggest:

let accessors = keys.filter(key => isAccessor(descriptors[key]));

if (accessors.length > 0) {
  // ...snip...
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why I run over the object twice is it's also part of the cloning process. We can't do extracted = assign({}, properties) because that'll actually trigger any native getters that exist there, we have to skip over native getters. This means we need to iterate the full list of properties.

However, we don't want to start cloning eagerly, and create a new object, unless one of the values in the object is an accessor. So we need to check to see if anything is an accessor first, then we need to loop over all the keys again to assign them conditionally, based on whether or not one is an accessor.

We could alternatively mutate the properties object in place' but in order to do that we would need to use Object.defineProperty since we're talking about overriding native getters/setters:

let accessors = keys.filter(key => isAccessor(descriptors[key]));

accessors.forEach(key => {
  Object.defineProperty(properties, key, { value: descriptors[key] });
})

I figured the double whammy of using Object.defineProperty (which I understand to be slow-ish) and mutating the object in place didn't make sense, but happy to change it if that makes more sense

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, yes I agree. Also, I was surprised that IE9+ has Array.prototype.some.


if (hasAccessors) {
let extracted = {};

keys.forEach(key => {
let descriptor = descriptors[key];

if (isAccessor(descriptor)) {
extracted[key] = nativeDescDecorator(descriptor);
} else {
extracted[key] = properties[key];
}
});

return extracted;
}
}

return properties;
}

const CONTINUE: MixinLike = {};

function mixinProperties<T extends MixinLike>(mixinsMeta: Meta, mixin: T): MixinLike {
Expand Down Expand Up @@ -550,7 +581,7 @@ export default class Mixin {
_without: any[] | undefined;

constructor(mixins: Mixin[] | undefined, properties?: { [key: string]: any }) {
this.properties = properties;
this.properties = extractAccessors(properties);
this.mixins = buildMixinsArray(mixins);
this.ownerConstructor = undefined;
this._without = undefined;
Expand Down
38 changes: 38 additions & 0 deletions packages/@ember/-internals/metal/tests/mixin/accessor_test.js
View file
Open in desktop
@@ -0,0 +1,38 @@
import { Mixin } from '../..';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';

moduleFor(
'Mixin Accessors',
class extends AbstractTestCase {
['@test works with getters'](assert) {
let count = 0;

let MixinA = Mixin.create({
get prop() {
return count++;
},
});

let obj = {};
MixinA.apply(obj);

assert.equal(obj.prop, 0, 'getter defined correctly');
assert.equal(obj.prop, 1, 'getter defined correctly');
}

['@test works with setters'](assert) {
let MixinA = Mixin.create({
set prop(value) {
this._prop = value + 1;
},
});

let obj = {};
MixinA.apply(obj);

obj.prop = 0;

assert.equal(obj._prop, 1, 'setter defined correctly');
}
}
);
1 change: 1 addition & 0 deletions packages/@ember/-internals/utils/index.ts
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
*/
export { default as symbol, isInternalSymbol } from './lib/symbol';
export { default as dictionary } from './lib/dictionary';
export { default as getOwnPropertyDescriptors } from './lib/get-own-property-descriptors';
export { uuid, GUID_KEY, generateGuid, guidFor } from './lib/guid';
export { default as intern } from './lib/intern';
export {
Expand Down
17 changes: 17 additions & 0 deletions packages/@ember/-internals/utils/lib/get-own-property-descriptors.ts
View file
Open in desktop
@@ -0,0 +1,17 @@
let getOwnPropertyDescriptors: (obj: { [x: string]: any }) => { [x: string]: PropertyDescriptor };

if (Object.getOwnPropertyDescriptors !== undefined) {
getOwnPropertyDescriptors = Object.getOwnPropertyDescriptors;
} else {
getOwnPropertyDescriptors = function(obj: object) {
let descriptors = {};

Object.keys(obj).forEach(key => {
descriptors[key] = Object.getOwnPropertyDescriptor(obj, key);
});

return descriptors;
};
}

export default getOwnPropertyDescriptors;