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] Allow accessors in mixins #17710

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Mar 6, 2019

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

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.
@pzuraq pzuraq requested a review from rwjblue March 6, 2019 15:56
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

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.

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.

@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 6, 2019

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
Copy link
Member

rwjblue commented Mar 6, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

rwjblue commented Mar 8, 2019

@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)

@rwjblue rwjblue merged commit d1506dc into master Mar 8, 2019
@rwjblue rwjblue deleted the bugfix/allow-accessors-in-mixins branch March 8, 2019 01:36
@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 8, 2019

results.pdf

For completeness, the benchmark of said application

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.volatile deprecation in 3.9 needs an alternative for "classic" classes
3 participants