From 6af84c45e949e414b834eee1479b10dd27f2980c Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Fri, 7 Apr 2017 19:14:55 -0700 Subject: [PATCH] Address feedback based on review. * PropertyAccessors must call `_flushProperties` to enable * Avoid tearing off oldProps (unnecessary) * Add `addBinding` docs * Merge notifyListeners into `setupBindings` * Add comment re: path-bindings not being overridable * Remove `dom` argument from `_bindTemplate` * Rename `_stampBoundTemplate` back to `_stampTemplate` --- lib/elements/dom-bind.html | 2 +- lib/mixins/element-mixin.html | 2 +- lib/mixins/property-accessors.html | 34 +++- lib/mixins/property-effects.html | 279 ++++++++++++++++------------- lib/utils/templatize.html | 2 +- test/unit/property-accessors.html | 3 + 6 files changed, 185 insertions(+), 137 deletions(-) diff --git a/lib/elements/dom-bind.html b/lib/elements/dom-bind.html index 8cd4487f8d..71486d026a 100644 --- a/lib/elements/dom-bind.html +++ b/lib/elements/dom-bind.html @@ -85,7 +85,7 @@ observer.observe(this, {childList: true}); return; } - this.root = this._stampBoundTemplate(template); + this.root = this._stampTemplate(template); this.$ = this.root.$; this.__children = []; for (let n=this.root.firstChild; n; n=n.nextSibling) { diff --git a/lib/mixins/element-mixin.html b/lib/mixins/element-mixin.html index b0b402cc16..29ee127f4e 100644 --- a/lib/mixins/element-mixin.html +++ b/lib/mixins/element-mixin.html @@ -622,7 +622,7 @@ ready() { if (this._template) { hostStack.beginHosting(this); - this.root = this._stampBoundTemplate(this._template); + this.root = this._stampTemplate(this._template); hostStack.endHosting(this); this.$ = this.root.$; } diff --git a/lib/mixins/property-accessors.html b/lib/mixins/property-accessors.html index e045d2ef15..b15568e4c8 100644 --- a/lib/mixins/property-accessors.html +++ b/lib/mixins/property-accessors.html @@ -79,7 +79,10 @@ * the standard `static get observedAttributes()`, implement `_propertiesChanged` * on the class, and then call `MyClass.createPropertiesForAttributes()` once * on the class to generate property accessors for each observed attribute - * prior to instancing. Any `observedAttributes` will automatically be + * prior to instancing. Last, call `this._flushProperties()` once to enable + * the accessors. + * + * Any `observedAttributes` will automatically be * deserialized via `attributeChangedCallback` and set to the associated * property using `dash-case`-to-`camelCase` convention. * @@ -137,6 +140,7 @@ this.__dataOld = null; if (this.__dataProto) { this._initializeProtoProperties(this.__dataProto); + this.__dataProto = null; } // Capture instance properties; these will be set into accessors // during first flush. Don't set them here, since we want @@ -168,6 +172,22 @@ } } + /** + * Called at ready time with bag of instance properties that overwrote + * accessors when the element upgraded. + * + * The default implementation sets these properties back into the + * setter at ready time. This method is provided as an override + * point for customizing or providing more efficient initialization. + * + * @param {Object} props Bag of property values that were overwritten + * when creating property accessors. + * @protected + */ + _initializeInstanceProperties(props) { + Object.assign(this, props); + } + /** * Ensures the element has the given attribute. If it does not, * assigns the given value to the attribute. @@ -444,7 +464,7 @@ * @protected */ _invalidateProperties() { - if (!this.__dataInvalid) { + if (!this.__dataInvalid && this.__dataInitialized) { this.__dataInvalid = true; microtask.run(() => { if (this.__dataInvalid) { @@ -460,17 +480,20 @@ * pending changes (and old values recorded when pending changes were * set), and resets the pending set of changes. * + * Note that this method must be called once to enable the property + * accessors system. For elements, generally `connectedCallback` + * is a normal spot to do so. + * * @protected */ _flushProperties() { if (!this.__dataInitialized) { this.ready() } else if (this.__dataPending) { - let oldProps = this.__dataOld; let changedProps = this.__dataPending; this.__dataPending = null; this.__dataCounter++; - this._propertiesChanged(this.__data, changedProps, oldProps); + this._propertiesChanged(this.__data, changedProps, this.__dataOld); this.__dataCounter--; } } @@ -493,7 +516,8 @@ // Update instance properties that shadowed proto accessors; these take // priority over any defaults set in constructor or attributeChangedCallback if (this.__dataInstanceProps) { - Object.assign(this, this.__dataInstanceProps); + this._initializeInstanceProperties(this.__dataInstanceProps); + this.__dataInstanceProps = null; } this.__dataInitialized = true; // Run normal flush diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 822cef13ca..435e558d4a 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -292,20 +292,6 @@ dispatchNotifyEvent(inst, info.eventName, value, path); } - /** - * Adds a 2-way binding notification event listener to the node specified - * - * @param {Object} node Child element to add listener to - * @param {Object} inst Host element instance to handle notification event - * @param {Object} info Listener metadata stored via addAnnotatedListener - * @private - */ - function addNotifyListener(node, inst, info) { - node.addEventListener(info.event, function(e) { - handleNotification(e, inst, info.target, info.source, info.negate); - }); - } - /** * Handler function for 2-way notification events. Receives context * information captured in the `addNotifyListener` closure from the @@ -448,51 +434,70 @@ // -- bindings ---------------------------------------------- /** - * Adds "binding" property effects for the template annotation - * ("note" for short) and node index specified. These may either be normal - * "binding" effects (property/path bindings) or "method binding" - * effects, aka inline computing functions, depending on the type of binding - * detailed in the note. + * Adds binding metadata to the current `nodeInfo`, and binding effects + * for all part dependencies to `templateInfo`. * - * @param {Object} model Prototype or instance - * @param {Object} note Annotation note returned from Annotator - * @param {number} index Index into `nodeList` that the binding applies to - * @param {Object=} dynamicFns Map indicating whether method names should - * be included as a dependency to the effect. + * @param {Function} constructor Class that `_parseTemplate` is currently + * running on + * @param {Object} templateInfo Template metadata for current template + * @param {Object} nodeInfo Node metadata for current template node + * @param {string} kind Binding kind, either 'property', 'attribute', or 'text' + * @param {string} target Target property name + * @param {Array} parts Array of binding part metadata + * @param {string} literal Literal text surrounding binding parts (specified + * only for 'property' bindings, since these must be initialized as part + * of boot-up) * @private */ function addBinding(constructor, templateInfo, nodeInfo, kind, target, parts, literal) { // Create binding metadata and add to nodeInfo nodeInfo.bindings = nodeInfo.bindings || []; let binding = { kind, target, parts, literal, isCompound: (parts.length !== 1) }; - let index = templateInfo.nodeInfoList.length; nodeInfo.bindings.push(binding); - // Add 2-way binding listener metadata to templateInfo + // Add listener info to binding metadata if (shouldAddListener(binding)) { - addAnnotatedListener(templateInfo, index, binding); + let {event, negate} = binding.parts[0]; + binding.listenerEvent = event || (CaseMap.camelToDashCase(target) + '-changed'); + binding.listenerNegate = negate; } // Add "propagate" property effects to templateInfo + let index = templateInfo.nodeInfoList.length; for (let i=0; i