[BUGFIX release-1-13] Ensure concatenatedProperties are not stomped. #12104
Conversation
|
Thanks for tracking this down, gents! |
| this._super(...arguments); | ||
|
|
||
| if (!this.constructor.__avoidPropagating) { | ||
| this.constructor.__avoidPropagating = dictionary(null); |
There was a problem hiding this comment.
@rwjblue this should likely be an EmptyObject (eventually)
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 emberjs#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
force-pushed
the
fix-concatenated-properties
branch
from
a92f60e
to
98d7cf3
Compare
|
Updated with some tweaks suggested by @stefanpenner in chat. |
|
@rwjblue Why |
|
@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. |
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) |
|
👍 Thanks. I'm going to look up some of these optimizations so I can have a little bit of a better understanding. :) |
[BUGFIX release-1-13] Ensure concatenatedProperties are not stomped.
|
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? |
|
@sukima - Yes, we plan to release a 1.13.x version with this fix. |
|
@sukima You can downgrade to 1.13.6 if you need immediate relief. If that is an option of course. |
|
@rwjblue and @rondale-sc thank you! |
When
_propagateAttrsToThisis invoked it iterates all properties that are present inthis.attrsand callsthis.set(attrName, attrValue). This is normally exactly what you expect. However, in the case ofconcatenatedPropertiesandmergedPropertiesthisseting 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, andactions). This was obviously a very naive check, and leaves any external usages ofconcatenatedPropertiesin components/views completely hosed.The changes here introduces a __avoidPropagating property that we can use to prevent
concatenatedPropertiesandmergedPropertiesfrom 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__avoidPropagatingon the constructor itself.One notable addon that has broken functionality is yapplabs/ember-modal-dialog#71.
Fixes #12099.
Implemented by @rondale-sc and @rwjblue.