Skip to content
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

Coerce undefined to default when binding. Fixes #1742. #1941

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/lib/bind/accessors.html
Original file line number Diff line number Diff line change
Expand Up @@ -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
_setProperty: function(property, value, quiet, node) {
_setProperty: function(property, value, fromAbove, node) {
node = node || this;
var effects = node._propertyEffects && node._propertyEffects[property];
if (effects) {
node._propertySetter(property, value, effects, quiet);
node._propertySetter(property, value, effects, fromAbove);
} else {
node[property] = value;
}
Expand Down
45 changes: 27 additions & 18 deletions src/standard/configure.html
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,20 @@

// storage for configuration
_setupConfigure: function(initialConfig) {
this._config = initialConfig || {};
this._inputConfig = initialConfig || {};
this._defaultConfig = {};
this._config = null;
this._handlers = [];
},

// static attributes are deserialized into _config
// static attributes are deserialized into _inputConfig
_marshalAttributes: function() {
this._takeAttributesToModel(this._config);
this._takeAttributesToModel(this._inputConfig);
},

// at configure time values are stored in _config
// at configure time values are stored in _inputConfig
_configValue: function(name, value) {
this._config[name] = value;
this._inputConfig[name] = value;
},

// Override polymer-mini thunk
Expand All @@ -71,20 +73,17 @@
// 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
// get individual default values from `value` in property configs
this.behaviors.forEach(function(b) {
this._configureProperties(b.properties, config);
// mixed-in behaviors
this._configureProperties(b.properties, this._defaultConfig);
}, this);
// prototypical behavior
this._configureProperties(this.properties, config);
// override local configuration with configuration from above
this._mixinConfigure(config, this._aboveConfig);
this._configureProperties(this.properties, this._defaultConfig);
// override default configuration with configuration from above
// this is the new _config, which are the final values to be applied
this._config = config;
this._config = Object.create(this._defaultConfig);
this._mixinConfigure(this._config, this._inputConfig);
// pass configuration data to bindings
this._distributeConfig(this._config);
},
Expand All @@ -103,6 +102,16 @@
}
},

/**
* Resets a property to its default if it exists, otherwise `undefined`.
*
* @method resetProperty
* @param {string} property Property to reset.
*/
resetProperty: function(property) {
this[property] = this._defaultConfig[property];
},

_mixinConfigure: function(a, b) {
for (var prop in b) {
if (!this.getPropertyInfo(prop).readOnly) {
Expand Down Expand Up @@ -138,21 +147,21 @@
_afterClientsReady: function() {
// process static effects, e.g. computations that have only literal arguments
this._executeStaticEffects();
this._applyConfig(this._config, this._aboveConfig);
this._applyConfig(this._config, this._inputConfig);
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, aboveConfig) {
_applyConfig: function(config, inputConfig) {
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; set quietly if value was
// configured from above, as opposed to default
this._setProperty(n, config[n], n in aboveConfig);
this._setProperty(n, config[n], n in inputConfig);
}
}
},
Expand Down
37 changes: 24 additions & 13 deletions src/standard/effectBuilder.html
Original file line number Diff line number Diff line change
Expand Up @@ -253,20 +253,31 @@
if (info.kind == 'attribute') {
this.serializeValueToAttribute(value, property, node);
} else {
// TODO(sorvell): consider pre-processing the following two string
// comparisons in the hot path so this can be a boolean check
if (property === 'className') {
value = this._scopeElementClass(node, value);
}
// Some browsers serialize `undefined` to `"undefined"`
if (property === 'textContent' ||
(node.localName == 'input' && property == 'value')) {
value = value == undefined ? '' : value;
// coerce undefined back to default
// TODO(kschaaf): coersion could be done in one of three places:
// 1. property setter (any propety set to undefined bounces to default)
// 2. all bindings (when binding, coerce undefined to the node's default)
// 3. only path bindings (where undefined most likely & problematic)
// we choose (3) because it is the most limited change
if (value === undefined && (info.model != property) &&
node.resetProperty) {
node.resetProperty(property);
} else {
// TODO(sorvell): consider pre-processing the following two string
// comparisons in the hot path so this can be a boolean check
if (property === 'className') {
value = this._scopeElementClass(node, value);
}
// Some browsers serialize `undefined` to `"undefined"`
if (property === 'textContent' ||
(node.localName == 'input' && property == 'value')) {
value = value == undefined ? '' : value;
}
// TODO(kschaaf): Ideally we'd use `fromAbove: true`, but this
// breaks read-only properties
// this._setPropertyQuiet(property, value, node);
node[property] = value;
}
// TODO(kschaaf): Ideally we'd use `fromAbove: true`, but this
// breaks read-only properties
// this._setProperty(property, value, true, node);
return node[property] = value;
}
},

Expand Down
17 changes: 10 additions & 7 deletions test/unit/notify-path-elements.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
notifyingValue: {
type: Number,
notify: true
},
valueWithDefault: {
type: Number,
value: 99
}
}
});
Expand Down Expand Up @@ -50,7 +54,7 @@
},
objValueChanged: function(value) {
this.observerCounts.objValueChanged++;
assert.equal(this.obj.value, value);
assert.equal(this.obj && this.obj.value, value);
},
});
</script>
Expand Down Expand Up @@ -95,15 +99,15 @@
},
objValueChanged: function(value) {
this.observerCounts.objValueChanged++;
assert.equal(this.obj.value, value);
assert.equal(this.obj && this.obj.value, value);
},
});
</script>
</dom-module>

<dom-module id="x-stuff">
<template>
<x-basic id="basic" notifying-value="{{nested.obj.value}}" attrvalue$="{{nested.obj.value}}"></x-basic>
<x-basic id="basic" notifying-value="{{nested.obj.value}}" value-with-default="{{nested.obj.value}}" attrvalue$="{{nested.obj.value}}"></x-basic>
<x-compose id="compose" obj="{{nested.obj}}"></x-compose>
<x-forward id="forward" obj="{{nested.obj}}"></x-forward>
<div id="boundChild" computed-from-paths="{{computeFromPaths(a, nested.b, nested.obj.c)}}" array-length="{{data.length}}"></div>
Expand Down Expand Up @@ -157,11 +161,11 @@
},
nestedObjChanged: function(value) {
this.observerCounts.nestedObjChanged++;
assert.equal(this.nested.obj, value);
assert.equal(this.nested && this.nested.obj, value);
},
nestedObjSubpathChanged: function(change) {
this.observerCounts.nestedObjSubpathChanged++;
assert.equal(change.base, this.nested.obj);
assert.equal(change.base, this.nested && this.nested.obj);
if (this.expectedNestedObjSubpath) {
assert.equal(change.path, this.expectedNestedObjSubpath);
assert.equal(change.value, this.expectedNestedObjValue);
Expand All @@ -175,12 +179,11 @@
this.observerCounts.multipleChanged++;
assert.equal(a, 'a');
assert.equal(b, 'b');
assert.equal(nestedObjChange.base, this.nested.obj);
assert.equal(nestedObjChange.base, this.nested && this.nested.obj);
if (this.expectedNestedObjSubpath) {
assert.equal(nestedObjChange.path, this.expectedNestedObjSubpath);
assert.equal(nestedObjChange.value, this.expectedNestedObjValue);
}
assert.equal(nestedObjChange.base, this.nested.obj);
},
computeFromPaths: function(a, b, c) {
assert.equal(a, this.a, 'computeFromNested `a` arg incorrect');
Expand Down
13 changes: 13 additions & 0 deletions test/unit/notify-path.html
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,19 @@
assert.equal(el.$.forward.$.compose.$.basic2.getAttribute('attrvalue'), '42');
});

test('reset to default on null', function() {
// Setup
var nested = {
obj: {
value: 42
}
};
el.nested = nested;
assert.equal(el.$.basic.valueWithDefault, 42);
el.nested = null;
assert.equal(el.$.basic.valueWithDefault, 99);
});

test('notification from basic element property change', function() {
// Setup
var nested = {
Expand Down