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 release-1-13] Ensure concatenatedProperties are not stomped. #12104

Merged
merged 1 commit into from Aug 15, 2015

Conversation

Projects
None yet
5 participants
@rwjblue
Member

rwjblue commented Aug 14, 2015

When _propagateAttrsToThis is invoked it iterates all properties that are present in this.attrs and calls this.set(attrName, attrValue). This is normally exactly what you expect. However, in the case of concatenatedProperties and mergedProperties this seting causes the concatenated / merged property to be completely clobbered.

The fix introduced in #12073 adds a very simple work around for the internally known items (just hard coding things like classNames, classNameBindings, attributeBindings, and actions). This was obviously a very naive check, and leaves any external usages of concatenatedProperties in components/views completely hosed.


The changes here introduces a __avoidPropagating property that we can use to prevent concatenatedProperties and mergedProperties from being set inside _propagateAttrsToThis. To avoid introducing this cost for every component created (when only classes can define new concatenated / merged properties) we are storing the __avoidPropagating on the constructor itself.

One notable addon that has broken functionality is yapplabs/ember-modal-dialog#71.


Fixes #12099.

Implemented by @rondale-sc and @rwjblue.

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Aug 14, 2015

Member

Thanks for tracking this down, gents!

Member

lukemelia commented Aug 14, 2015

Thanks for tracking this down, gents!

@stefanpenner

View changes

Show outdated Hide outdated packages/ember-views/lib/compat/attrs-proxy.js
[BUGFIX release-1-13] Ensure concatenatedProperties are not stomped.
When `_propagateAttrsToThis` is invoked it iterates all properties that
are present in `this.attrs` and calls `this.set(attrName, attrValue)`.
This is normally exactly what you expect. However, in the case of
`concatenatedProperties` and `mergedProperties` this `set`ing causes the
concatenated / merged property to be completely clobbered.

The fix introduced in #12073 adds a very simple work around for the
internally known items (just hard coding things like `classNames`,
`classNameBindings`, `attributeBindings`, and `actions`). This was
obviously a very naive check, and leaves any external usages of
`concatenatedProperties` in components/views completely hosed.

---

The changes here introduces a __avoidPropagating property that we can
use to prevent `concatenatedProperties` and `mergedProperties` from
being set inside `_propagateAttrsToThis`. To avoid introducing this cost
for every component created (when only classes can define new
concatenated / merged properties) we are storing the
`__avoidPropagating` on the constructor itself.
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 14, 2015

Member

Updated with some tweaks suggested by @stefanpenner in chat.

Member

rwjblue commented Aug 14, 2015

Updated with some tweaks suggested by @stefanpenner in chat.

@rondale-sc

This comment has been minimized.

Show comment
Hide comment
@rondale-sc

rondale-sc Aug 14, 2015

Contributor

@rwjblue Why EmptyObject over dictionary in this case?

Contributor

rondale-sc commented Aug 14, 2015

@rwjblue Why EmptyObject over dictionary in this case?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 14, 2015

Member

@stefanpenner explained it to me like this in chat:

Use dictionary for long lived dictionary's that will have many add / removes over time.

EmptyObject is like 10x faster than Object.create(null) (which is done by dictionary) and is better suited for this kind of thing because we aren't actually going to delete entries.

Member

rwjblue commented Aug 14, 2015

@stefanpenner explained it to me like this in chat:

Use dictionary for long lived dictionary's that will have many add / removes over time.

EmptyObject is like 10x faster than Object.create(null) (which is done by dictionary) and is better suited for this kind of thing because we aren't actually going to delete entries.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 14, 2015

Member

EmptyObject is like 10x faster than Object.create(null) (which is done by dictionary) and is better suited for this kind of thing because we aren't actually going to delete entries.

Dictionary is for things that will most likely be deleted from

dictionary could also use EmptyObject, but its also the deletion that makes it slow (although it would be less slow once moved to EmptyObject)

Member

stefanpenner commented Aug 14, 2015

EmptyObject is like 10x faster than Object.create(null) (which is done by dictionary) and is better suited for this kind of thing because we aren't actually going to delete entries.

Dictionary is for things that will most likely be deleted from

dictionary could also use EmptyObject, but its also the deletion that makes it slow (although it would be less slow once moved to EmptyObject)

@rondale-sc

This comment has been minimized.

Show comment
Hide comment
@rondale-sc

rondale-sc Aug 14, 2015

Contributor

👍 Thanks. I'm going to look up some of these optimizations so I can have a little bit of a better understanding. :)

Contributor

rondale-sc commented Aug 14, 2015

👍 Thanks. I'm going to look up some of these optimizations so I can have a little bit of a better understanding. :)

rwjblue added a commit that referenced this pull request Aug 15, 2015

Merge pull request #12104 from rwjblue/fix-concatenated-properties
[BUGFIX release-1-13] Ensure concatenatedProperties are not stomped.

@rwjblue rwjblue merged commit b53366a into emberjs:master Aug 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the rwjblue:fix-concatenated-properties branch Aug 15, 2015

@sukima

This comment has been minimized.

Show comment
Hide comment
@sukima

sukima Aug 20, 2015

Contributor

Will this be part of a bug fix release on version 1.13? This is a show stopper as the expectation declared in the documentation is broken. Many people (including us) can not upgrade to 2.0 yet.

If not can we have a work around best practice to handle the broken behavior till we can upgrade to 2.0?

Contributor

sukima commented Aug 20, 2015

Will this be part of a bug fix release on version 1.13? This is a show stopper as the expectation declared in the documentation is broken. Many people (including us) can not upgrade to 2.0 yet.

If not can we have a work around best practice to handle the broken behavior till we can upgrade to 2.0?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 20, 2015

Member

@sukima - Yes, we plan to release a 1.13.x version with this fix.

Member

rwjblue commented Aug 20, 2015

@sukima - Yes, we plan to release a 1.13.x version with this fix.

@rondale-sc

This comment has been minimized.

Show comment
Hide comment
@rondale-sc

rondale-sc Aug 20, 2015

Contributor

@sukima You can downgrade to 1.13.6 if you need immediate relief. If that is an option of course.

Contributor

rondale-sc commented Aug 20, 2015

@sukima You can downgrade to 1.13.6 if you need immediate relief. If that is an option of course.

@sukima

This comment has been minimized.

Show comment
Hide comment
@sukima

sukima Aug 24, 2015

Contributor

@rwjblue and @rondale-sc thank you!

Contributor

sukima commented Aug 24, 2015

@rwjblue and @rondale-sc thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment