From 1c2d10a7e0d23444cd33a3f229124832bc9666db Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Fri, 14 Aug 2015 16:35:06 -0400 Subject: [PATCH] [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. (cherry picked from commit 98d7cf327f0c2ce97eadd9b7dfefbbb51734e1eb) --- .../integration/component_invocation_test.js | 23 +++++++++++ .../ember-views/lib/compat/attrs-proxy.js | 38 +++++++++++++++---- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/packages/ember-htmlbars/tests/integration/component_invocation_test.js b/packages/ember-htmlbars/tests/integration/component_invocation_test.js index 2ba11b0b63c..5566029b0d1 100644 --- a/packages/ember-htmlbars/tests/integration/component_invocation_test.js +++ b/packages/ember-htmlbars/tests/integration/component_invocation_test.js @@ -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')) { diff --git a/packages/ember-views/lib/compat/attrs-proxy.js b/packages/ember-views/lib/compat/attrs-proxy.js index 73ce9e231df..44282d395fe 100644 --- a/packages/ember-views/lib/compat/attrs-proxy.js +++ b/packages/ember-views/lib/compat/attrs-proxy.js @@ -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.`; @@ -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; } @@ -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)); } }