Skip to content

Commit

Permalink
Disambiguate config fromAbove and local defaults, pass to _setProperty.
Browse files Browse the repository at this point in the history
- Fixes bug in previous fix for #1839/#1854 where notifying properties would not notify during configuration
- renames `_setPropertyQuiet` to `_setProperty` with `quiet` arg
- renames `_propertySet` to `_propertySetter` for clarity
  • Loading branch information
kevinpschaaf committed Jun 22, 2015
1 parent 19cdf42 commit c7fec1c
Show file tree
Hide file tree
Showing 9 changed files with 33 additions and 20 deletions.
8 changes: 4 additions & 4 deletions src/lib/bind/accessors.html
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@

// Called from accessors, where effects is pre-stored
// in the closure for the accessor for efficiency
_propertySet: function(property, value, effects, fromAbove) {
_propertySetter: function(property, value, effects, fromAbove) {
var old = this.__data__[property];
// NaN is always not equal to itself,
// if old and value are both NaN we treat them as equal
Expand All @@ -73,11 +73,11 @@
// TODO(kschaaf): downward bindings (e.g. _applyEffectValue) should also
// use non-notifying setters but right now that would require looking
// up readOnly property config in the hot-path
_setPropertyQuiet: function(property, value, node) {
_setProperty: function(property, value, quiet, node) {
node = node || this;
var effects = node._propertyEffects && node._propertyEffects[property];
if (effects) {
node._propertySet(property, value, effects, true);
node._propertySetter(property, value, effects, quiet);
} else {
node[property] = value;
}
Expand Down Expand Up @@ -169,7 +169,7 @@
}
};
var setter = function(value) {
this._propertySet(property, value, effects);
this._propertySetter(property, value, effects);
};
// ReadOnly properties have a private setter only
// TODO(kschaaf): Per current Bind factoring, we shouldn't
Expand Down
10 changes: 5 additions & 5 deletions src/lib/template/dom-repeat.html
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,9 @@
if (!row) {
this.rows.push(row = this._insertRow(i, null, item));
}
row._setPropertyQuiet(this.as, item);
row._setPropertyQuiet('__key__', key);
row._setPropertyQuiet(this.indexAs, i);
row._setProperty(this.as, item, true);
row._setProperty('__key__', key, true);
row._setProperty(this.indexAs, i, true);
}
// Remove extra
for (; i<this.rows.length; i++) {
Expand Down Expand Up @@ -569,7 +569,7 @@
_forwardParentProp: function(prop, value) {
if (this.rows) {
this.rows.forEach(function(row) {
row._setPropertyQuiet(prop, value);
row._setProperty(prop, value, true);
}, this);
}
},
Expand Down Expand Up @@ -598,7 +598,7 @@
path = this.as + '.' + path.substring(dot+1);
row.notifyPath(path, value, true);
} else {
row._setPropertyQuiet(this.as, value);
row._setProperty(this.as, value, true);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/lib/template/templatizer.html
Original file line number Diff line number Diff line change
Expand Up @@ -206,14 +206,14 @@
},

// Similar to Polymer.Base.extend, but retains any previously set instance
// values (_propertySet back on instance once accessor is installed)
// values (_propertySetter back on instance once accessor is installed)
_extendTemplate: function(template, proto) {
Object.getOwnPropertyNames(proto).forEach(function(n) {
var val = template[n];
var pd = Object.getOwnPropertyDescriptor(proto, n);
Object.defineProperty(template, n, pd);
if (val !== undefined) {
template._propertySet(n, val);
template._propertySetter(n, val);
}
});
},
Expand Down
16 changes: 9 additions & 7 deletions src/standard/configure.html
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@
// e.g. hand template content stored in notes to children as part of
// configure flow so templates have their content at ready time
this._configureAnnotationReferences();
// save copy of configuration that came from above
this._aboveConfig = this.mixin({}, this._config);
// get individual default values from property configs
var config = {};
// mixed-in behaviors
Expand All @@ -79,9 +81,8 @@
}, this);
// prototypical behavior
this._configureProperties(this.properties, config);
// get add'l default values from central configure
// combine defaults returned from configure with inputs in _config
this._mixinConfigure(config, this._config);
// override local configuration with configuration from above
this._mixinConfigure(config, this._aboveConfig);
// this is the new _config, which are the final values to be applied
this._config = config;
// pass configuration data to bindings
Expand Down Expand Up @@ -137,20 +138,21 @@
_afterClientsReady: function() {
// process static effects, e.g. computations that have only literal arguments
this._executeStaticEffects();
this._applyConfig(this._config);
this._applyConfig(this._config, this._aboveConfig);
this._flushHandlers();
},

// NOTE: values are already propagated to children via
// _distributeConfig so propagation triggered by effects here is
// redundant, but safe due to dirty checking
_applyConfig: function(config) {
_applyConfig: function(config, aboveConfig) {
for (var n in config) {
// Don't stomp on values that may have been set by other side effects
if (this[n] === undefined) {
// Call _propertySet for any properties with accessors, which will
// initialize read-only properties also
this._setPropertyQuiet(n, config[n]);
// initialize read-only properties also; set quietly if value was
// configured from above, as opposed to default
this._setProperty(n, config[n], n in aboveConfig);
}
}
},
Expand Down
2 changes: 1 addition & 1 deletion src/standard/effectBuilder.html
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@
}
// TODO(kschaaf): Ideally we'd use `fromAbove: true`, but this
// breaks read-only properties
// this._setPropertyQuiet(property, value, node);
// this._setProperty(property, value, true, node);
return node[property] = value;
}
},
Expand Down
2 changes: 1 addition & 1 deletion src/standard/notify-path.html
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
a dirty check of whether the new value was already known
*/
notifyPath: function(path, value, fromAbove) {
var old = this._propertySet(path, value);
var old = this._propertySetter(path, value);
// manual dirty checking for now...
// NaN is always not equal to itself,
// if old and value are both NaN we treat them as equal
Expand Down
2 changes: 2 additions & 0 deletions src/standard/utils.html
Original file line number Diff line number Diff line change
Expand Up @@ -310,11 +310,13 @@
* @method mixin
* @param {Object} target Target object to copy properties to.
* @param {Object} source Source object to copy properties from.
* @return {Object} Target object that was passed as first argument.
*/
mixin: function(target, source) {
for (var i in source) {
target[i] = source[i];
}
return target;
}

});
Expand Down
5 changes: 5 additions & 0 deletions test/unit/bind-elements.html
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@
notify: true,
observer: 'notifyingvalueChanged'
},
notifyingvalueWithDefault: {
notify: true,
value: 99
},
computednotifyingvalue: {
type: Number,
notify: true,
Expand Down Expand Up @@ -232,6 +236,7 @@
<x-basic id="basic1"
value="{{boundvalue}}"
notifyingvalue="{{boundnotifyingvalue}}"
notifyingvalue-with-default="{{boundnotifyingvalueWithDefault}}"
camel-notifying-value="{{boundnotifyingvalue}}"
computedvalue="{{boundcomputedvalue}}"
computednotifyingvalue="{{boundcomputednotifyingvalue}}"
Expand Down
4 changes: 4 additions & 0 deletions test/unit/bind.html
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,10 @@
assert.equal(el.boundnotifyingvalue, -43, 'camel-case binding to notifying property not updated');
});

test('binding to notifying property with default', function() {
assert.equal(el.boundnotifyingvalueWithDefault, 99);
});

test('observer for property bound to notifying property', function() {
el.$.basic1.notifyingvalue = 45;
assert.equal(el.observerCounts.boundnotifyingvalueChanged, 1, 'observer for property bound to notifying property not called');
Expand Down

0 comments on commit c7fec1c

Please sign in to comment.