Skip to content

Commit

Permalink
[BUGFIX release-1-13] Ensure concatenatedProperties are not stomped.
Browse files Browse the repository at this point in the history
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.

(cherry picked from commit 98d7cf3)
  • Loading branch information
rwjblue committed Aug 24, 2015
1 parent 3aacc67 commit 1c2d10a
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,29 @@ QUnit.test('specifying classNames results in correct class', function(assert) {
ok(button.is('.foo.bar.baz.ember-view'), 'the element has the correct classes: ' + button.attr('class'));
});

QUnit.test('specifying custom concatenatedProperties avoids clobbering', function(assert) {
expect(1);

let clickyThing;
registry.register('component:some-clicky-thing', Component.extend({
concatenatedProperties: ['blahzz'],
blahzz: ['blark', 'pory'],
init() {
this._super(...arguments);
clickyThing = this;
}
}));

view = EmberView.extend({
template: compile('{{#some-clicky-thing blahzz="baz"}}Click Me{{/some-clicky-thing}}'),
container: container
}).create();

runAppend(view);

assert.deepEqual(clickyThing.get('blahzz'), ['blark', 'pory', 'baz'], 'property is properly combined');
});

// jscs:disable validateIndentation
if (Ember.FEATURES.isEnabled('ember-htmlbars-component-generation')) {

Expand Down
38 changes: 30 additions & 8 deletions packages/ember-views/lib/compat/attrs-proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Mixin } from 'ember-metal/mixin';
import { symbol } from 'ember-metal/utils';
import { PROPERTY_DID_CHANGE } from 'ember-metal/property_events';
import { on } from 'ember-metal/events';
import EmptyObject from 'ember-metal/empty_object';

export function deprecation(key) {
return `You tried to look up an attribute directly on the component. This is deprecated. Use attrs.${key} instead.`;
Expand All @@ -13,9 +14,37 @@ function isCell(val) {
return val && val[MUTABLE_CELL];
}

function setupAvoidPropagating(instance) {
// This caches the list of properties to avoid setting onto the component instance
// inside `_propagateAttrsToThis`. We cache them so that every instantiated component
// does not have to pay the calculation penalty.
let constructor = instance.constructor;
if (!constructor.__avoidPropagating) {
constructor.__avoidPropagating = new EmptyObject();
let i, l;
for (i = 0, l = instance.concatenatedProperties.length; i < l; i++) {
let prop = instance.concatenatedProperties[i];

constructor.__avoidPropagating[prop] = true;
}

for (i = 0, l = instance.mergedProperties.length; i < l; i++) {
let prop = instance.mergedProperties[i];

constructor.__avoidPropagating[prop] = true;
}
}
}

let AttrsProxyMixin = {
attrs: null,

init() {
this._super(...arguments);

setupAvoidPropagating(this);
},

getAttr(key) {
let attrs = this.attrs;
if (!attrs) { return; }
Expand All @@ -42,14 +71,7 @@ let AttrsProxyMixin = {
let attrs = this.attrs;

for (let prop in attrs) {
if (prop !== 'attrs' &&
// These list of properties are concatenated and merged properties of
// Ember.View / Ember.Component. Setting them here results in them being
// completely stomped and not handled properly, BAIL OUT!
prop !== 'actions' &&
prop !== 'classNames' &&
prop !== 'classNameBindings' &&
prop !== 'attributeBindings') {
if (prop !== 'attrs' && !this.constructor.__avoidPropagating[prop]) {
this.set(prop, this.getAttr(prop));
}
}
Expand Down

0 comments on commit 1c2d10a

Please sign in to comment.