[BUGFIX beta] Allow accessors in mixins

[pzuraq]

This fix allows native object accessors to work in Ember Mixins. It does
this by looping over the properties of a new mixin and extracting their
descriptors, checking to see if any of them are accessors. If they are,
it wraps them in a descriptor decorator.

Benchmarked against Ember Observer, and didn't find any statistically significant changes:

results.pdf

Partially fixes #17709, the deprecation app needs to be updated still

[rwjblue]

Wasn't the idea to only do this at the top level? If I read this code correctly, it would allow it for mixins and the POJO passed to .extend.

[rwjblue]

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?

[pzuraq]

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

[rwjblue]

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

[pzuraq]

I figured that fixing it for .extend() and not for mixins would be somewhat buggy, and counter to people's expectations. It would definitely be possible to do it that way though, we can change it if you think that makes sense.

[rwjblue]

I figured that fixing it for .extend() and not for mixins would be somewhat buggy, and counter to people's expectations.

Yeah, I understand this for sure, but I think its fairly easy to teach that the getter/setter support is only for .extend and not .create.

I'm mostly worried about what happens in a much larger app with many more mixins/classes and so I'm looking for a way to limit the impact even further (only supporting accessors in .extend would significantly limit the number of times this code path is hit), but frankly that may not be warranted.

[krisselden]

We need to test in another app. I agree with it in extend only but that implies Mixin.create to me, I agree it should be in emberobject.create.

[krisselden]

Also while overall it looked insignificant the boot phase looked significant. I think it warrants some further investigation. We risk overfitting a small app by never using anything else but emberobserver

[rwjblue]

@pzuraq tested in a large application, there was a very small regression (~0.83%) which we think is acceptable given the fact that we are actively transitioning away from this codepath (towards native classes)

[pzuraq]

results.pdf

For completeness, the benchmark of said application