From ea4e7d970869881248d1b79b86baf19598838e2f Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Thu, 6 Apr 2017 02:33:03 -0700 Subject: [PATCH 01/17] Improvements to binding API: - Adds override points for _parseBindings and _evaluateBinding - Adds support for runtime template binding - Moves ready(), _hasAccessor tracking, and instance property swizzle at ready time to PropertyAccessors --- lib/elements/dom-bind.html | 3 +- lib/mixins/element-mixin.html | 9 +- lib/mixins/property-accessors.html | 86 ++- lib/mixins/property-effects.html | 863 +++++++++++++++-------------- lib/mixins/template-stamp.html | 35 +- lib/utils/templatize.html | 2 +- test/unit/templatize.html | 4 +- 7 files changed, 540 insertions(+), 462 deletions(-) diff --git a/lib/elements/dom-bind.html b/lib/elements/dom-bind.html index d08ca0e37c..b91438c364 100644 --- a/lib/elements/dom-bind.html +++ b/lib/elements/dom-bind.html @@ -85,8 +85,7 @@ observer.observe(this, {childList: true}); return; } - this._bindTemplate(template); - this.root = this._stampTemplate(template); + this.root = this._stampBoundTemplate(template); this.__children = []; for (let n=this.root.firstChild; n; n=n.nextSibling) { this.__children[this.__children.length] = n; diff --git a/lib/mixins/element-mixin.html b/lib/mixins/element-mixin.html index 30af1ec1a7..17e1287264 100644 --- a/lib/mixins/element-mixin.html +++ b/lib/mixins/element-mixin.html @@ -420,7 +420,7 @@ if (window.ShadyCSS) { window.ShadyCSS.prepareTemplate(template, is, ext); } - proto._bindTemplate(template, propertiesForClass(proto.constructor)); + proto._bindTemplate(template); } function flushPropertiesStub() {} @@ -620,7 +620,7 @@ ready() { if (this._template) { hostStack.beginHosting(this); - this.root = this._stampTemplate(this._template); + this.root = this._stampBoundTemplate(this._template); hostStack.endHosting(this); } super.ready(); @@ -745,6 +745,11 @@ return Polymer.ResolveUrl.resolveUrl(url, base); } + static _parseTemplateContent(template, templateInfo, nodeInfo) { + templateInfo.dynamicFns = templateInfo.dynamicFns || propertiesForClass(this); + return super._parseTemplateContent(template, templateInfo, nodeInfo); + } + } return PolymerElement; diff --git a/lib/mixins/property-accessors.html b/lib/mixins/property-accessors.html index 8d1c03074e..e045d2ef15 100644 --- a/lib/mixins/property-accessors.html +++ b/lib/mixins/property-accessors.html @@ -129,6 +129,7 @@ _initializeProperties() { this.__serializing = false; this.__dataCounter = 0; + this.__dataInitialized = false; this.__dataInvalid = false; // initialize data with prototype values saved when creating accessors this.__data = {}; @@ -137,6 +138,16 @@ if (this.__dataProto) { this._initializeProtoProperties(this.__dataProto); } + // Capture instance properties; these will be set into accessors + // during first flush. Don't set them here, since we want + // these to overwrite defaults/constructor assignments + for (let p in this.__dataHasAccessor) { + if (this.hasOwnProperty(p)) { + this.__dataInstanceProps = this.__dataInstanceProps || {}; + this.__dataInstanceProps[p] = this[p]; + delete this[p]; + } + } } /** @@ -347,15 +358,31 @@ * @protected */ _createPropertyAccessor(property, readOnly) { - saveAccessorValue(this, property); - Object.defineProperty(this, property, { - get: function() { - return this.__data[property]; - }, - set: readOnly ? function() { } : function(value) { - this._setProperty(property, value); - } - }); + if (!this.hasOwnProperty('__dataHasAccessor')) { + this.__dataHasAccessor = Object.assign({}, this.__dataHasAccessor); + } + if (!this.__dataHasAccessor[property]) { + this.__dataHasAccessor[property] = true; + saveAccessorValue(this, property); + Object.defineProperty(this, property, { + get: function() { + return this.__data[property]; + }, + set: readOnly ? function() { } : function(value) { + this._setProperty(property, value); + } + }); + } + } + + /** + * Returns true if this library created an accessor for the given property. + * + * @param {string} property Property name + * @return {boolean} True if an accessor was created + */ + _hasAccessor(property) { + return this.__dataHasAccessor && this.__dataHasAccessor[property]; } /** @@ -436,12 +463,41 @@ * @protected */ _flushProperties() { - let oldProps = this.__dataOld; - let changedProps = this.__dataPending; - this.__dataPending = null; - this.__dataCounter++; - this._propertiesChanged(this.__data, changedProps, oldProps); - this.__dataCounter--; + 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.__dataCounter--; + } + } + + /** + * Lifecycle callback called the first time properties are being flushed. + * Prior to `ready`, all property sets through accessors are queued and + * their effects are flushed after this method returns. + * + * Users may override this function to implement behavior that is + * dependent on the element having its properties initialized, e.g. + * from defaults (initialized from `constructor`, `_initializeProperties`), + * `attributeChangedCallback`, or values propagated from host e.g. via + * bindings. `super.ready()` must be called to ensure the data system + * becomes enabled. + * + * @public + */ + ready() { + // 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.__dataInitialized = true; + // Run normal flush + this._flushProperties(); } /** diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 2de01ab393..0d4e023b31 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -32,7 +32,6 @@ // Property effect types; effects are stored on the prototype using these keys const TYPES = { - ANY: '__propertyEffects', COMPUTE: '__computeEffects', REFLECT: '__reflectEffects', NOTIFY: '__notifyEffects', @@ -90,14 +89,16 @@ * @param {string} type Type of effect to run * @param {Object} props Bag of current property changes * @param {Object=} oldProps Bag of previous values for changed properties + * @param {boolean} hasPaths True with `props` contains one or more paths + * @param {*} var_args Additional metadata to pass to effect function * @private */ - function runEffects(inst, effects, props, oldProps, hasPaths) { + function runEffects(inst, effects, props, oldProps, hasPaths, ...args) { if (effects) { let ran; let id = dedupeId++; for (let prop in props) { - if (runEffectsForProperty(inst, effects, id, prop, props, oldProps, hasPaths)) { + if (runEffectsForProperty(inst, effects, id, prop, props, oldProps, hasPaths, ...args)) { ran = true; } } @@ -110,13 +111,15 @@ * * @param {Object} inst The instance with effects to run * @param {Array} effects Array of effects - * @param {number} id Effect run id used for de-duping effects + * @param {number} dedupeId Counter used for de-duping effects * @param {string} prop Name of changed property - * @param {*} value Value of changed property - * @param {*} old Previous value of changed property + * @param {*} props Changed properties + * @param {*} oldProps Old properties + * @param {boolean} hasPaths True with `props` contains one or more paths + * @param {*} var_args Additional metadata to pass to effect function * @private */ - function runEffectsForProperty(inst, effects, dedupeId, prop, props, oldProps, hasPaths) { + function runEffectsForProperty(inst, effects, dedupeId, prop, props, oldProps, hasPaths, ...args) { let ran; let rootProperty = hasPaths ? Polymer.Path.root(prop) : prop; let fxs = effects[rootProperty]; @@ -124,10 +127,10 @@ for (let i=0, l=fxs.length, fx; (i-changed') * @param {Object} inst Host element instance handling the notification event - * @param {string} property Child element property that was bound - * @param {string} path Host property/path that was bound + * @param {string} fromProp Child element property that was bound + * @param {string} toPath Host property/path that was bound * @param {boolean} negate Whether the binding was negated * @private */ - function handleNotification(e, inst, property, path, negate) { + function handleNotification(event, inst, fromProp, toPath, negate) { let value; - let targetPath = e.detail && e.detail.path; - if (targetPath) { - path = Polymer.Path.translate(property, path, targetPath); - value = e.detail && e.detail.value; + let detail = event.detail; + let fromPath = detail && detail.path; + if (fromPath) { + toPath = Polymer.Path.translate(fromProp, toPath, fromPath); + value = detail && detail.value; } else { - value = e.target[property]; + value = event.target[fromProp]; } value = negate ? !value : value; - setPropertyFromNotification(inst, path, value, e); - } - - /** - * Called by 2-way binding notification event listeners to set a property - * or path to the host based on a notification from a bound child. - * - * @param {string} path Path on this instance to set - * @param {*} value Value to set to given path - * @protected - */ - function setPropertyFromNotification(inst, path, value, event) { - let detail = event.detail; - if (detail && detail.queueProperty) { - if (!inst.__readOnly || !inst.__readOnly[path]) { - inst._setPendingPropertyOrPath(path, value, true, Boolean(detail.path)); + if (!inst.__readOnly || !inst.__readOnly[toPath]) { + if (inst._setPendingPropertyOrPath(toPath, value, true, Boolean(fromPath)) + && (!detail || !detail.queueProperty)) { + inst._invalidateProperties(); } - } else { - inst.set(path, value); } } @@ -382,6 +374,7 @@ * @param {Element} inst The instance the effect will be run on * @param {Object} changedProps Bag of changed properties * @param {Object} oldProps Bag of previous values for changed properties + * @param {boolean} hasPaths True with `props` contains one or more paths * @private */ function runComputedEffects(inst, changedProps, oldProps, hasPaths) { @@ -412,7 +405,7 @@ function runComputedEffect(inst, property, props, oldProps, info) { let result = runMethodEffect(inst, property, props, oldProps, info); let computedProp = info.methodInfo; - if (inst.__propertyEffects && inst.__propertyEffects[computedProp]) { + if (inst.__dataHasAccessor && inst.__dataHasAccessor[computedProp]) { inst._setPendingProperty(computedProp, result, true); } else { inst[computedProp] = result; @@ -425,6 +418,7 @@ * * @param {Element} inst The instance whose props are changing * @param {Object} changedProps Bag of changed properties + * @param {boolean} hasPaths True with `props` contains one or more paths * @private */ function computeLinkedPaths(inst, changedProps, hasPaths) { @@ -453,6 +447,16 @@ // -- bindings ---------------------------------------------- + function addBinding(nodeInfo, kind, target, parts, literal) { + nodeInfo.bindings = nodeInfo.bindings || []; + let binding = { + kind, target, parts, literal, + isCompound: (parts.length !== 1) + }; + nodeInfo.bindings.push(binding); + return binding; + } + /** * Adds "binding" property effects for the template annotation * ("note" for short) and node index specified. These may either be normal @@ -462,37 +466,36 @@ * * @param {Object} model Prototype or instance * @param {Object} note Annotation note returned from Annotator - * @param {number} index Index into `__templateNodes` list of annotated nodes that the - * note applies to + * @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. * @private */ - function addBindingEffect(model, note, index, dynamicFns) { - for (let i=0; i info.value.length) && - (info.kind == 'property') && !info.isCompound && - node.__propertyEffects && node.__propertyEffects[info.name]) { + if (hasPaths && part.source && (path.length > part.source.length) && + (binding.kind == 'property') && !binding.isCompound && + node.__dataHasAccessor && node.__dataHasAccessor[binding.target]) { let value = props[path]; - path = Polymer.Path.translate(info.value, info.name, path); + path = Polymer.Path.translate(part.source, binding.target, path); if (node._setPendingPropertyOrPath(path, value, false, true)) { inst._enqueueClient(node); } } else { - // Root or deeper path was set; extract bound path value - // e.g.: foo="{{obj.sub}}", path: 'obj', set 'foo'=obj.sub - // or: foo="{{obj.sub}}", path: 'obj.sub.prop', set 'foo'=obj.sub - if (path != info.value) { - value = Polymer.Path.get(inst, info.value); - } else { - if (hasPaths && Polymer.Path.isPath(path)) { - value = Polymer.Path.get(inst, path); - } else { - value = inst.__data[path]; - } - } + let value = info.evaluator._evaluateBinding(inst, part, path, props, oldProps, hasPaths); // Propagate value to child - applyBindingValue(inst, info, value); + applyBindingValue(inst, node, binding, part, value); } } @@ -548,19 +544,18 @@ * @param {*} value Value to set * @private */ - function applyBindingValue(inst, info, value) { - let node = inst.__templateNodes[info.index]; - value = computeBindingValue(node, value, info); + function applyBindingValue(inst, node, binding, part, value) { + value = computeBindingValue(node, value, binding, part); if (Polymer.sanitizeDOMValue) { - value = Polymer.sanitizeDOMValue(value, info.name, info.kind, node); + value = Polymer.sanitizeDOMValue(value, binding.target, binding.kind, node); } - if (info.kind == 'attribute') { + if (binding.kind == 'attribute') { // Attribute binding - inst._valueToNodeAttribute(node, value, info.name); + inst._valueToNodeAttribute(node, value, binding.target); } else { // Property binding - let prop = info.name; - if (node.__propertyEffects && node.__propertyEffects[prop]) { + let prop = binding.target; + if (node.__dataHasAccessor && node.__dataHasAccessor[prop]) { if (!node.__readOnly || !node.__readOnly[prop]) { if (node._setPendingProperty(prop, value)) { inst._enqueueClient(node); @@ -578,74 +573,26 @@ * * @param {Node} node Node the value will be set to * @param {*} value Value to set - * @param {Object} info Effect metadata + * @param {Object} binding Binding metadata * @return {*} Transformed value to set * @private */ - function computeBindingValue(node, value, info) { - if (info.negate) { - value = !value; - } - if (info.isCompound) { - let storage = node.__dataCompoundStorage[info.name]; - storage[info.compoundIndex] = value; + function computeBindingValue(node, value, binding, part) { + if (binding.isCompound) { + let storage = node.__dataCompoundStorage[binding.target]; + storage[part.compoundIndex] = value; value = storage.join(''); } - if (info.kind !== 'attribute') { + if (binding.kind !== 'attribute') { // Some browsers serialize `undefined` to `"undefined"` - if (info.name === 'textContent' || - (node.localName == 'input' && info.name == 'value')) { + if (binding.target === 'textContent' || + (node.localName == 'input' && binding.target == 'value')) { value = value == undefined ? '' : value; } } return value; } - /** - * Adds "binding method" property effects for the template binding - * ("note" for short), part metadata, and node index specified. - * - * @param {Object} model Prototype or instance - * @param {Object} note Binding note returned from Annotator - * @param {Object} part The compound part metadata - * @param {number} index Index into `__templateNodes` list of annotated nodes that the - * note applies to - * @param {Object=} dynamicFns Map indicating whether method names should - * be included as a dependency to the effect. - * @private - */ - function addMethodBindingEffect(model, note, part, index, dynamicFns) { - createMethodEffect(model, part.signature, TYPES.PROPAGATE, - runMethodBindingEffect, { - index: index, - isCompound: note.isCompound, - compoundIndex: part.compoundIndex, - kind: note.kind, - name: note.name, - negate: part.negate, - part: part - }, dynamicFns - ); - } - - /** - * Implements the "binding method" (inline computed function) effect. - * - * Runs the method with the values of the arguments specified in the `info` - * object and setting the return value to the node property/attribute. - * - * @param {Object} inst The instance the effect will be run on - * @param {string} property Name of property - * @param {*} value Current value of property - * @param {*} old Previous value of property - * @param {Object} info Effect metadata - * @private - */ - function runMethodBindingEffect(inst, property, props, oldProps, info) { - let val = runMethodEffect(inst, property, props, oldProps, info); - applyBindingValue(inst, info.methodInfo, val); - } - /** * Returns true if a binding's metadata meets all the requirements to allow * 2-way binding, and therefore a -changed event listener should be @@ -660,7 +607,7 @@ * @private */ function shouldAddListener(binding) { - return binding.name && + return binding.target && binding.kind != 'attribute' && binding.kind != 'text' && !binding.isCompound && @@ -672,60 +619,47 @@ * instance time to add event listeners for 2-way bindings. * * @param {Object} model Prototype (instances not currently supported) - * @param {number} index Index into `__templateNodes` list of annotated nodes that the - * event should be added to - * @param {string} property Property of target node to listen for changes - * @param {string} path Host path that the change should be propagated to - * @param {string=} event A custom event name to listen for (e.g. via the - * `{{prop::eventName}}` syntax) - * @param {boolean=} negate Whether the notified value should be negated before - * setting to host path - * @private - */ - function addAnnotatedListener(model, index, property, path, event, negate) { - event = event || (CaseMap.camelToDashCase(property) + '-changed'); - model.__notifyListeners = model.__notifyListeners || []; - model.__notifyListeners.push({ index, property, path, event, negate }); - } - - /** - * Adds all 2-way binding notification listeners to a host based on - * `__notifyListeners` metadata recorded by prior calls to`addAnnotatedListener` - * - * @param {Object} inst Host element instance + * @param {number} index Index into `nodeList` that the event should be added to + * @param {Object} binding Binding metadata * @private */ - function setupNotifyListeners(inst) { - let b$ = inst.__notifyListeners; - for (let i=0, l=b$.length, info; (i lastIndex) { - parts.push({literal: text.slice(lastIndex, m.index)}); - } - // Add binding part - // Mode (one-way or two) - let mode = m[1][0]; - let negate = Boolean(m[2]); - let value = m[3].trim(); - let customEvent, event, colon; - if (mode == '{' && (colon = value.indexOf('::')) > 0) { - event = value.substring(colon + 2); - value = value.substring(0, colon); - customEvent = true; - } - let signature = parseMethod(value, hostProps); - let rootProperty; - if (!signature) { - rootProperty = Polymer.Path.root(value); - hostProps[rootProperty] = true; - } - parts.push({ - value, mode, negate, event, customEvent, signature, rootProperty, - compoundIndex: parts.length - }); - lastIndex = bindingRegex.lastIndex; - } - // Add a final literal part - if (lastIndex && lastIndex < text.length) { - let literal = text.substring(lastIndex); - if (literal) { - parts.push({ literal }); - } - } - if (parts.length) { - return parts; - } - } - function literalFromParts(parts) { let s = ''; for (let i=0; i= 0) { + effects.splice(idx, 1); + } + } + /** * Returns whether the current prototype/instance has a property effect * of a certain type. @@ -1296,7 +1173,7 @@ * @protected */ _hasPropertyEffect(property, type) { - let effects = this[type || TYPES.ANY]; + let effects = this[type]; return Boolean(effects && effects[property]); } @@ -1378,9 +1255,9 @@ */ _setPendingPropertyOrPath(path, value, shouldNotify, isPathNotification) { let rootProperty = Polymer.Path.root(Array.isArray(path) ? path[0] : path); - let hasEffect = this.__propertyEffects && this.__propertyEffects[rootProperty]; + let hasAccessor = this.__dataHasAccessor && this.__dataHasAccessor[rootProperty]; let isPath = (rootProperty !== path); - if (hasEffect) { + if (hasAccessor) { if (isPath) { if (!isPathNotification) { // Dirty check changes being set to a path against the actual object, @@ -1590,55 +1467,14 @@ } /** - * Overrides PropertyAccessor's default async queuing of - * `_propertiesChanged`, to instead synchronously flush - * `_propertiesChanged` unless the `this._asyncEffects` property is true. - * - * If this is the first time properties are being flushed, the `ready` - * callback will be called. + * Overrides PropertyAccessor * * @override */ - _flushProperties() { - if (!this.__dataInitialized) { - this.ready() - } else if (this.__dataPending) { - super._flushProperties(); - if (!this.__dataCounter) { - // Clear temporary cache at end of turn - this.__dataTemp = {}; - } - } - } - - /** - * Polymer-specific lifecycle callback called the first time properties - * are being flushed. Prior to `ready`, all property sets through - * accessors are queued and their effects are flushed after this method - * returns. - * - * Users may override this function to implement behavior that is - * dependent on the element having its properties initialized, e.g. - * from defaults (initialized from `constructor`, `_initializeProperties`), - * `attributeChangedCallback`, or binding values propagated from host - * "binding effects". `super.ready()` must be called to ensure the - * data system becomes enabled. - * - * @public - */ ready() { - // Update instance properties that shadowed proto accessors; these take - // priority over any defaults set in `properties` or constructor - let instanceProps = this.__dataInstanceProps; - if (instanceProps) { - initalizeInstanceProperties(this, instanceProps); - } - // Enable acceessors - this.__dataInitialized = true; - if (this.__dataPending) { - // Run normal flush - this._flushProperties(); - } else { + let dataPending = this.__dataPending; + super.ready(); + if (!dataPending) { this._readyClients(); } } @@ -1654,30 +1490,6 @@ this.__dataClientsInitialized = true; } - /** - * Stamps the provided template and performs instance-time setup for - * Polymer template features, including data bindings, declarative event - * listeners, and the `this.$` map of `id`'s to nodes. A document fragment - * is returned containing the stamped DOM, ready for insertion into the - * DOM. - * - * Note that for host data to be bound into the stamped DOM, the template - * must have been previously bound to the prototype via a call to - * `_bindTemplate`, which performs one-time template binding work. - * - * Note that this method currently only supports being called once per - * instance. - * - * @param {HTMLTemplateElement} template Template to stamp - * @return {DocumentFragment} Cloned template content - * @protected - */ - _stampTemplate(template) { - let dom = super._stampTemplate(template); - setupBindings(this, template._templateInfo.nodeInfoList); - return dom; - } - /** * Implements `PropertyAccessors`'s properties changed callback. * @@ -1703,7 +1515,7 @@ let notifyProps = this.__dataToNotify; this.__dataToNotify = null; // Propagate properties to clients - runEffects(this, this.__propagateEffects, changedProps, oldProps, hasPaths); + this._propagatePropertyChanges(changedProps, oldProps, hasPaths); // Flush clients this._flushClients(); // Reflect properties @@ -1714,11 +1526,36 @@ if (notifyProps) { runNotifyEffects(this, notifyProps, changedProps, oldProps, hasPaths); } + // Clear temporary cache at end of turn + if (this.__dataCounter == 1) { + this.__dataTemp = {}; + } // ---------------------------- // window.debug && console.groupEnd(this.localName + '#' + this.id + ': ' + c); // ---------------------------- } + /** + * Called to propagate any property changes to stamped template nodes + * managed by this element. + * + * @param {Object} changedProps Bag of changed properties + * @param {Object} oldProps Bag of previous values for changed properties + * @param {boolean} hasPaths True with `props` contains one or more paths + * @protected + */ + _propagatePropertyChanges(changedProps, oldProps, hasPaths) { + if (this.__propagateEffects) { + runEffects(this, this.__propagateEffects, changedProps, oldProps, hasPaths); + } + let templateInfo = this.__templateInfo; + while (templateInfo) { + runEffects(this, templateInfo.propertyEffects, changedProps, oldProps, + hasPaths, templateInfo.nodeList); + templateInfo = templateInfo.nextTemplateInfo; + } + } + /** * Aliases one data path as another, such that path notifications from one * are routed to the other. @@ -2141,37 +1978,104 @@ // -- binding ---------------------------------------------- /** - * Creates "binding" property effects for all binding bindings - * in the provided template that forward host properties into DOM stamped - * from the template via `_stampTemplate`. + * Parses the provided template to ensure binding effects are created + * for them, and then ensures property accessors are created for any + * dependent properties in the template. Binding effects for bound + * templates are stored in a linked list on the instance so that + * templates can be efficiently stamped and unstamped. + * + * This method may be called on the prototype (for prototypical template + * binding) once per prototype, or on the instance for either the + * prototypically bound template and/or one or more templates to stamp + * bound to the instance. * * @param {HTMLTemplateElement} template Template containing binding * bindings - * @param {Object=} dynamicFns Map indicating whether method names should - * be included as a dependency to the effect. + * @param {DocumentFragment=} dom Stamped DOM for this template to be + * bound to the instance (leave undefined for prototypical template + * binding) + * @return {Object} Template metadata object * @protected */ - _bindTemplate(template, dynamicFns) { - // Clear any existing propagation effects inherited from superClass - this.__propagateEffects = {}; - this.__notifyListeners = []; + _bindTemplate(template, dom) { let templateInfo = this.constructor._parseTemplate(template); - let nodeInfo = templateInfo.nodeInfoList; - for (let i=0, info; (i} Array of binding part metadata + * @protected + */ + static _parseBindings(text, templateInfo) { + let parts = []; + let lastIndex = 0; + let m; + // Example: "literal1{{prop}}literal2[[!compute(foo,bar)]]final" + // Regex matches: + // Iteration 1: Iteration 2: + // m[1]: '{{' '[[' + // m[2]: '' '!' + // m[3]: 'prop' 'compute(foo,bar)' + while ((m = bindingRegex.exec(text)) !== null) { + // Add literal part + if (m.index > lastIndex) { + parts.push({literal: text.slice(lastIndex, m.index)}); + } + // Add binding part + let mode = m[1][0]; + let negate = Boolean(m[2]); + let source = m[3].trim(); + let customEvent, notifyEvent, colon; + if (mode == '{' && (colon = source.indexOf('::')) > 0) { + notifyEvent = source.substring(colon + 2); + source = source.substring(0, colon); + customEvent = true; + } + let signature = parseMethod(source); + let dependencies = []; + if (signature) { + // Inline computed function + let {args, methodName} = signature; + for (let i=0; i.content * is removed and stored in notes as well. * - * Note that this method may only be called once per instance (it does - * not support stamping multiple templates per element instance). - * * @param {HTMLTemplateElement} template Template to stamp * @return {DocumentFragment} Cloned template content */ @@ -406,10 +407,10 @@ let dom = document.importNode(content, true); // NOTE: ShadyDom optimization indicating there is an insertion point dom.__noInsertionPoint = !templateInfo.hasInsertionPoint; + let nodes = dom.nodeList = new Array(nodeInfo.length); this.$ = {}; - this.__templateNodes = new Array(nodeInfo.length); - for (let i=0, l=nodeInfo.length, info, node; (i Date: Thu, 6 Apr 2017 11:20:34 -0700 Subject: [PATCH 02/17] Fix _hasAccessor for readOnly. Collapse addBinding & addBindingEffects --- lib/mixins/element-mixin.html | 4 +++- lib/mixins/property-effects.html | 29 +++++++++++------------------ 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/lib/mixins/element-mixin.html b/lib/mixins/element-mixin.html index 17e1287264..0e48f7897d 100644 --- a/lib/mixins/element-mixin.html +++ b/lib/mixins/element-mixin.html @@ -576,7 +576,9 @@ let value = typeof info.value == 'function' ? info.value.call(this) : info.value; - if (this._hasPropertyEffect(p)) { + // Set via `_setProperty` if there is an accessor, to enable + // initializing readOnly property defaults + if (this._hasAccessor(p)) { this._setProperty(p, value) } else { this[p] = value; diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 0d4e023b31..2e5f1d85c4 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -447,16 +447,6 @@ // -- bindings ---------------------------------------------- - function addBinding(nodeInfo, kind, target, parts, literal) { - nodeInfo.bindings = nodeInfo.bindings || []; - let binding = { - kind, target, parts, literal, - isCompound: (parts.length !== 1) - }; - nodeInfo.bindings.push(binding); - return binding; - } - /** * Adds "binding" property effects for the template annotation * ("note" for short) and node index specified. These may either be normal @@ -471,10 +461,17 @@ * be included as a dependency to the effect. * @private */ - function addBindingEffects(constructor, templateInfo, binding, index) { + 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 if (shouldAddListener(binding)) { addAnnotatedListener(templateInfo, index, binding); } + // Add "propagate" property effects to templateInfo for (let i=0; i Date: Thu, 6 Apr 2017 15:12:08 -0700 Subject: [PATCH 03/17] Put $ on dom, and assign to element as needed. Eliminate _templateInfo reference. --- lib/elements/dom-bind.html | 1 + lib/mixins/element-mixin.html | 9 +++++++++ lib/mixins/property-effects.html | 25 ++++++++++++++++++++++--- lib/mixins/template-stamp.html | 9 +++++---- lib/utils/templatize.html | 22 ++++++++++------------ 5 files changed, 47 insertions(+), 19 deletions(-) diff --git a/lib/elements/dom-bind.html b/lib/elements/dom-bind.html index b91438c364..8cd4487f8d 100644 --- a/lib/elements/dom-bind.html +++ b/lib/elements/dom-bind.html @@ -86,6 +86,7 @@ return; } this.root = this._stampBoundTemplate(template); + this.$ = this.root.$; this.__children = []; for (let n=this.root.firstChild; n; n=n.nextSibling) { this.__children[this.__children.length] = n; diff --git a/lib/mixins/element-mixin.html b/lib/mixins/element-mixin.html index 0e48f7897d..b0b402cc16 100644 --- a/lib/mixins/element-mixin.html +++ b/lib/mixins/element-mixin.html @@ -624,6 +624,7 @@ hostStack.beginHosting(this); this.root = this._stampBoundTemplate(this._template); hostStack.endHosting(this); + this.$ = this.root.$; } super.ready(); } @@ -747,6 +748,14 @@ return Polymer.ResolveUrl.resolveUrl(url, base); } + /** + * Overrides `PropertyAccessors` to add map of dynamic functions on + * template info, for consumption by `PropertyEffects` template binding + * code. This map determines which method templates should have accessors + * created for them. + * + * @override + */ static _parseTemplateContent(template, templateInfo, nodeInfo) { templateInfo.dynamicFns = templateInfo.dynamicFns || propertiesForClass(this); return super._parseTemplateContent(template, templateInfo, nodeInfo); diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 2e5f1d85c4..822cef13ca 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -1130,7 +1130,7 @@ * its prototype, the property effect lists will be cloned and added as * own properties of the caller. * - * @param {string} path Property that should trigger the effect + * @param {string} property Property that should trigger the effect * @param {string} type Effect type, from this.PROPERTY_EFFECT_TYPES * @param {Object=} effect Effect metadata object * @protected @@ -2015,12 +2015,23 @@ return this.__templateInfo = templateInfo; } - static _addTemplatePropertyEffect(templateInfo, prop, info) { + /** + * Adds a property effect to the given template metadata, which is run + * at the "propagate" stage of `_propertiesChanged` when the template + * has been bound to the element via `_bindTemplate`. + * + * The `effect` object should match the format in `_addPropertyEffect`. + * + * @param {string} prop Property that should trigger the effect + * @param {Object=} effect Effect metadata object + * @protected + */ + static _addTemplatePropertyEffect(templateInfo, prop, effect) { let hostProps = templateInfo.hostProps = templateInfo.hostProps || {}; hostProps[prop] = true; let effects = templateInfo.propertyEffects = templateInfo.propertyEffects || {}; let propEffects = effects[prop] = effects[prop] || []; - propEffects.push(info); + propEffects.push(effect); } /** @@ -2030,6 +2041,14 @@ * is returned containing the stamped DOM, ready for insertion into the * DOM. * + * This method may be called more than once; however note that due to + * `shadycss` polyfill limitations, only styles from templates prepared + * using `ShadyCSS.prepareTemplate` will be correctly polyfilled (scoped + * to the shadow root and support CSS custom properties), and note that + * `ShadyCSS.prepareTemplate` may only be called once per element. As such, + * any styles required by in runtime-stamped templates must be included + * in the main element template. + * * @param {HTMLTemplateElement} template Template to stamp * @return {DocumentFragment} Cloned template content * @protected diff --git a/lib/mixins/template-stamp.html b/lib/mixins/template-stamp.html index 290b0ed793..7123f156c4 100644 --- a/lib/mixins/template-stamp.html +++ b/lib/mixins/template-stamp.html @@ -381,8 +381,9 @@ * The template is parsed (once and memoized) using this library's * template parsing features, and provides the following value-added * features: - * * Adds declarative event listners for `on-event="handler"` attributes - * * Generates an "id map" for all nodes with id's under `this.$` + * * Adds declarative event listeners for `on-event="handler"` attributes + * * Generates an "id map" for all nodes with id's under `$` on returned + * document fragment * * Passes template info including `content` back to templates as * `_templateInfo` (a performance optimization to avoid deep template * cloning) @@ -408,10 +409,10 @@ // NOTE: ShadyDom optimization indicating there is an insertion point dom.__noInsertionPoint = !templateInfo.hasInsertionPoint; let nodes = dom.nodeList = new Array(nodeInfo.length); - this.$ = {}; + dom.$ = {}; for (let i=0, l=nodeInfo.length, info; (i can only be templatized once'); } template.__templatizeOwner = owner; - let templateInfo = template._templateInfo; + let templateInfo = owner.constructor._parseTemplate(template); // Get memoized base class for the prototypical template, which // includes property effects for binding template & forwarding - let baseClass = templateInfo && templateInfo.templatizeInstanceClass; + let baseClass = templateInfo.templatizeInstanceClass; if (!baseClass) { - baseClass = createTemplatizerClass(template, options); - templateInfo = template._templateInfo; + baseClass = createTemplatizerClass(template, templateInfo, options); templateInfo.templatizeInstanceClass = baseClass; } // Host property forwarding must be installed onto template instance - addPropagateEffects(template, options); + addPropagateEffects(template, templateInfo, options); // Subclass base class and add reference for this specific template let klass = class TemplateInstance extends baseClass {}; klass.prototype._methodHost = findMethodHost(template); From 6af84c45e949e414b834eee1479b10dd27f2980c Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Fri, 7 Apr 2017 19:14:55 -0700 Subject: [PATCH 04/17] 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 Date: Mon, 10 Apr 2017 18:04:13 -0700 Subject: [PATCH 05/17] Add initial runtime stamping tests. --- test/runner.html | 1 + test/unit/property-effects-template.html | 398 +++++++++++++++++++++++ 2 files changed, 399 insertions(+) create mode 100644 test/unit/property-effects-template.html diff --git a/test/runner.html b/test/runner.html index 9a9f1b003a..8069589384 100644 --- a/test/runner.html +++ b/test/runner.html @@ -27,6 +27,7 @@ 'unit/globals.html', 'unit/property-accessors.html', 'unit/property-effects.html', + 'unit/property-effects-template.html', 'unit/path-effects.html', 'unit/shady.html', 'unit/shady-events.html', diff --git a/test/unit/property-effects-template.html b/test/unit/property-effects-template.html new file mode 100644 index 0000000000..6fa880bb20 --- /dev/null +++ b/test/unit/property-effects-template.html @@ -0,0 +1,398 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 1cf955b9e5eed73d40a511946b3a94b1a60dcac0 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 11 Apr 2017 18:07:41 -0700 Subject: [PATCH 06/17] Added tests for custom parsing, effects, and binding. --- test/unit/property-effects-template.html | 312 ++++++++++++++++++++++- 1 file changed, 299 insertions(+), 13 deletions(-) diff --git a/test/unit/property-effects-template.html b/test/unit/property-effects-template.html index 6fa880bb20..ea2b7f8198 100644 --- a/test/unit/property-effects-template.html +++ b/test/unit/property-effects-template.html @@ -32,8 +32,9 @@ + + + + + + + + From 14711067e25dc6bf72f8019e29705b4055023ff9 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 11 Apr 2017 18:36:58 -0700 Subject: [PATCH 07/17] Add tests for adding/removing runtime property effects. --- test/unit/property-effects-template.html | 14 ++++++ test/unit/property-effects.html | 59 ++++++++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/test/unit/property-effects-template.html b/test/unit/property-effects-template.html index ea2b7f8198..777b1e43ad 100644 --- a/test/unit/property-effects-template.html +++ b/test/unit/property-effects-template.html @@ -554,6 +554,20 @@ assertAllPropValues(el, sd, ld, 'path', 'obj.path+++', 4); }); + test('prototypical stamping not affected by runtime stamping', () => { + assertStampingCorrect(el, el.$); + let stamped = el.shadowRoot.querySelectorAll('x-element#first'); + assert.equal(stamped.length, 1); + assert.equal(stamped[0], el.$.first); + // Lifecycle order correct + assert.deepEqual(Polymer.lifecycleOrder.ready, [ + 'x-runtime|x-element#proto|x-element-child#noBinding', + 'x-runtime|x-element#proto|x-element-child#hasBinding', + 'x-runtime|x-element#proto', + 'x-runtime' + ]); + }); + }); suite('template parsing hooks', () => { diff --git a/test/unit/property-effects.html b/test/unit/property-effects.html index e59a6e2a92..3de0bfb660 100644 --- a/test/unit/property-effects.html +++ b/test/unit/property-effects.html @@ -1400,6 +1400,65 @@ }); +suite('runtime effects', function() { + + var el; + + setup(function() { + el = document.createElement('x-basic'); + document.body.appendChild(el); + }); + + teardown(function() { + document.body.removeChild(el); + }); + + test('add/remove runtime property effect', function() { + assert.equal(el.observerCounts.valueChanged, 1); + let info = {}; + let fn = sinon.spy(); + let effect = { info, fn }; + el._addPropertyEffect('value', el.PROPERTY_EFFECT_TYPES.OBSERVE, effect); + el.value = 'value++'; + assert.equal(el.observerCounts.valueChanged, 2); + assert.equal(fn.callCount, 1); + assert.equal(fn.firstCall.args[0], el); + assert.equal(fn.firstCall.args[1], 'value'); + assert.equal(fn.firstCall.args[2].value, 'value++'); + assert.equal(fn.firstCall.args[4], info); + el._removePropertyEffect('value', el.PROPERTY_EFFECT_TYPES.OBSERVE, effect); + el.value = 'value+++'; + assert.equal(fn.callCount, 1); + }); + + test('add/remove runtime path effect', function() { + assert.equal(el.observerCounts.valueChanged, 1); + let info = {}; + let fn = sinon.spy(); + let trigger = {name: 'value.path', structured: true}; + let effect = { info, fn, trigger }; + el._addPropertyEffect('value', el.PROPERTY_EFFECT_TYPES.OBSERVE, effect); + el.value = {path: 'value.path'}; + assert.equal(el.observerCounts.valueChanged, 2); + assert.equal(fn.callCount, 1); + assert.equal(fn.getCall(0).args[0], el); + assert.equal(fn.getCall(0).args[1], 'value'); + assert.equal(fn.getCall(0).args[2].value, el.value); + assert.equal(fn.getCall(0).args[4], info); + el.set('value.path', 'value.path++'); + assert.equal(el.observerCounts.valueChanged, 2); + assert.equal(fn.callCount, 2); + assert.equal(fn.getCall(1).args[0], el); + assert.equal(fn.getCall(1).args[1], 'value.path'); + assert.equal(fn.getCall(1).args[2]['value.path'], 'value.path++'); + assert.equal(fn.getCall(1).args[4], info); + el._removePropertyEffect('value', el.PROPERTY_EFFECT_TYPES.OBSERVE, effect); + el.set('value.path', 'value.path+++'); + assert.equal(fn.callCount, 2); + }); + +}); + From bf2dbe0a68956e1360544ae7cd3d013648bbf05e Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 11 Apr 2017 18:38:43 -0700 Subject: [PATCH 08/17] Ensure prototype wasn't affected by runtime effects. --- test/unit/property-effects.html | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/unit/property-effects.html b/test/unit/property-effects.html index 3de0bfb660..d42d1704e5 100644 --- a/test/unit/property-effects.html +++ b/test/unit/property-effects.html @@ -1402,7 +1402,7 @@ suite('runtime effects', function() { - var el; + var el, el2; setup(function() { el = document.createElement('x-basic'); @@ -1411,6 +1411,7 @@ teardown(function() { document.body.removeChild(el); + document.body.removeChild(el2); }); test('add/remove runtime property effect', function() { @@ -1429,6 +1430,10 @@ el._removePropertyEffect('value', el.PROPERTY_EFFECT_TYPES.OBSERVE, effect); el.value = 'value+++'; assert.equal(fn.callCount, 1); + // Ensure prototype wasn't affected + el2 = document.createElement('x-basic'); + document.body.appendChild(el2); + assert.equal(fn.callCount, 1); }); test('add/remove runtime path effect', function() { @@ -1455,6 +1460,10 @@ el._removePropertyEffect('value', el.PROPERTY_EFFECT_TYPES.OBSERVE, effect); el.set('value.path', 'value.path+++'); assert.equal(fn.callCount, 2); + // Ensure prototype wasn't affected + el2 = document.createElement('x-basic'); + document.body.appendChild(el2); + assert.equal(fn.callCount, 2); }); }); From dff5f2bcae033a56c5aedd95f31a175a033ca215 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 12 Apr 2017 15:40:05 -0700 Subject: [PATCH 09/17] Fix lint error. --- test/unit/property-effects-template.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/property-effects-template.html b/test/unit/property-effects-template.html index 777b1e43ad..5e0a855793 100644 --- a/test/unit/property-effects-template.html +++ b/test/unit/property-effects-template.html @@ -586,6 +586,7 @@ for (let i=0; i Date: Wed, 12 Apr 2017 18:17:50 -0700 Subject: [PATCH 10/17] Address feedback from review: * Refactor `_bindTemplate` to remove problematic `hasCreatedAccessors` * Remove vestigial `dom` from `_bindTemplate` call * Rename `_unstampTemplate` to `_removeBoundDom` * Add `infoIndex` to `nodeInfo` (and renamed parent & index) * Add test to ensure runtime accessors created for new props in runtime stamped template * Changed custom binding test to use different prop names * Added test for #first count after removing bound dom --- lib/mixins/property-effects.html | 49 ++++++++-------- lib/mixins/template-stamp.html | 16 +++--- test/unit/property-effects-template.html | 71 +++++++++++++++--------- 3 files changed, 80 insertions(+), 56 deletions(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 435e558d4a..06ef7eeded 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -645,7 +645,6 @@ addNotifyListener(node, inst, binding); } } - addNotifyListener node.__dataHost = inst; } } @@ -1999,35 +1998,40 @@ * templates can be efficiently stamped and unstamped. * * This method may be called on the prototype (for prototypical template - * binding) once per prototype, or on the instance for either the - * prototypically bound template and/or one or more templates to stamp - * bound to the instance. - * - * Note that a template may only be stamped by one element; stamping - * the same template by multiple element types is not currently supported. + * binding, to avoid creating accessors every instance) once per prototype, + * and will be called with `runtimeBinding: true` by `_stampTemplate` to + * create and link an instance of the template metadata associated with a + * particular stamping. * * @param {HTMLTemplateElement} template Template containing binding * bindings - * @param {DocumentFragment=} dom Stamped DOM for this template to be - * bound to the instance (leave undefined for prototypical template - * binding) - * @return {Object} Instanced template metadata object + * @param {boolean=} runtimeBinding When false (default), performs + * "prototypical" binding of the template and overwrites any previously + * bound template for the class. When true (as passed from + * `_stampTemplate`), the template info is instanced and linked into + * the list of bound templates. + * @return {Object} Template metadata object; for `runtimeBinding`, + * this is an instance of the prototypical template info * @protected */ - _bindTemplate(template) { + _bindTemplate(template, instanceBinding) { let templateInfo = this.constructor._parseTemplate(template); - if (!templateInfo.hasCreatedAccessors) { - // Create accessors for any template effects + let wasPreBound = this.__templateInfo == templateInfo; + // Optimization: since this is called twice for proto-bound templates, + // don't attempt to recreate accessors if this template was pre-bound + if (!wasPreBound) { for (let prop in templateInfo.propertyEffects) { this._createPropertyAccessor(prop); } - templateInfo.hasCreatedAccessors = true; } - // Create instance of template metadata and link into list of templates - templateInfo = Object.create(templateInfo); - if (this.hasOwnProperty('__templateInfo')) { - templateInfo.nextTemplateInfo = this.__templateInfo; - this.__templateInfo.previousTemplateInfo = templateInfo; + if (instanceBinding) { + // For instance-time binding, create instance of template metadata + // and link into list of templates if necessary + templateInfo = Object.create(templateInfo); + if (!wasPreBound && this.__templateInfo) { + templateInfo.nextTemplateInfo = this.__templateInfo; + this.__templateInfo.previousTemplateInfo = templateInfo; + } } return this.__templateInfo = templateInfo; } @@ -2072,8 +2076,7 @@ */ _stampTemplate(template) { let dom = super._stampTemplate(template); - // Link template info & create accessors - let templateInfo = this._bindTemplate(template, dom); + let templateInfo = this._bindTemplate(template, true); // Add template-instance-specific data to instanced templateInfo templateInfo.nodeList = dom.nodeList; templateInfo.childNodes = Array.from(dom.childNodes); @@ -2096,7 +2099,7 @@ * from `_stampTemplate` associated with the nodes to be removed * @protected */ - _unstampTemplate(dom) { + _removeBoundDom(dom) { // Unlink template info let templateInfo = dom.templateInfo; if (templateInfo.previousTemplateInfo) { diff --git a/lib/mixins/template-stamp.html b/lib/mixins/template-stamp.html index 7123f156c4..50cf1783dc 100644 --- a/lib/mixins/template-stamp.html +++ b/lib/mixins/template-stamp.html @@ -45,13 +45,13 @@ function findTemplateNode(root, nodeInfo) { // recursively ascend tree until we hit root - let parent = nodeInfo.parent && findTemplateNode(root, nodeInfo.parent); + let parent = nodeInfo.parentInfo && findTemplateNode(root, nodeInfo.parentInfo); // unwind the stack, returning the indexed node at each level if (parent) { // note: marginally faster than indexing via childNodes // (http://jsperf.com/childnodes-lookup) for (let n=parent.firstChild, i=0; n; n=n.nextSibling) { - if (nodeInfo.index === i++) { + if (nodeInfo.parentIndex === i++) { return n; } } @@ -152,8 +152,8 @@ * templateInfo: , * // Metadata to allow efficient retrieval of instanced node * // corresponding to this metadata - * parent: , - * index: + * parentInfo: , + * parentIndex: * }, * ... * ], @@ -248,7 +248,7 @@ * @param {Object} nodeInfo Node metadata for current template. */ static _parseTemplateChildNodes(root, templateInfo, nodeInfo) { - for (let node=root.firstChild, index=0, next; node; node=next) { + for (let node=root.firstChild, parentIndex=0, next; node; node=next) { // Wrap templates if (node.localName == 'template') { node = wrapTemplateExtension(node); @@ -272,11 +272,11 @@ continue; } } - let childInfo = { index, parent: nodeInfo }; + let childInfo = { parentIndex, parentInfo: nodeInfo }; if (this._parseTemplateNode(node, templateInfo, childInfo)) { - templateInfo.nodeInfoList.push(childInfo); + childInfo.infoIndex = templateInfo.nodeInfoList.push(childInfo) - 1; } - index++; + parentIndex++; } } diff --git a/test/unit/property-effects-template.html b/test/unit/property-effects-template.html index 5e0a855793..216958367c 100644 --- a/test/unit/property-effects-template.html +++ b/test/unit/property-effects-template.html @@ -103,6 +103,9 @@ + @@ -231,35 +233,37 @@ From 0724f1870855fbfaffe69e9ac92cc8b6606ed5b2 Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Thu, 13 Apr 2017 11:32:17 -0700 Subject: [PATCH 12/17] use chrome beta --- .travis.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 6465d133dd..fc6c1e13e7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,11 +8,12 @@ addons: sources: - google-chrome packages: - - google-chrome-stable + - google-chrome-beta cache: directories: - node_modules before_script: +- alias google-chrome google-chrome-beta - npm install -g bower gulp-cli@1 - bower install - gulp lint From d297047eb460e24a7d2e3e855cb1bede1b01628b Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Thu, 13 Apr 2017 11:36:58 -0700 Subject: [PATCH 13/17] alias another way --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index fc6c1e13e7..cdb0129741 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,7 +13,9 @@ cache: directories: - node_modules before_script: -- alias google-chrome google-chrome-beta +- mkdir -p ~/bin +- ln -s /usr/bin/google-chrome-beta ~/bin/google-chrome +- export PATH=$HOME/bin:$PATH - npm install -g bower gulp-cli@1 - bower install - gulp lint From becd1d3bcebd907d264809a7e33477312a3013a9 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Thu, 13 Apr 2017 11:33:13 -0700 Subject: [PATCH 14/17] [ci skip] Fix comment. --- lib/mixins/property-effects.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 06ef7eeded..93d1bdeaeb 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -2005,7 +2005,7 @@ * * @param {HTMLTemplateElement} template Template containing binding * bindings - * @param {boolean=} runtimeBinding When false (default), performs + * @param {boolean=} instanceBinding When false (default), performs * "prototypical" binding of the template and overwrites any previously * bound template for the class. When true (as passed from * `_stampTemplate`), the template info is instanced and linked into From d0bd96d4c4008f696db70d835db7f1543ab3414f Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Thu, 13 Apr 2017 11:59:47 -0700 Subject: [PATCH 15/17] Add more comments --- lib/mixins/template-stamp.html | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/mixins/template-stamp.html b/lib/mixins/template-stamp.html index 50cf1783dc..67031ceea4 100644 --- a/lib/mixins/template-stamp.html +++ b/lib/mixins/template-stamp.html @@ -154,6 +154,7 @@ * // corresponding to this metadata * parentInfo: , * parentIndex: + * infoIndex: Date: Thu, 13 Apr 2017 15:56:48 -0700 Subject: [PATCH 16/17] Fix perf regressions. --- lib/mixins/property-effects.html | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 93d1bdeaeb..f02a24b6db 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -692,10 +692,12 @@ * @private */ function addNotifyListener(node, inst, binding) { - let part = binding.parts[0]; - node.addEventListener(binding.listenerEvent, function(e) { - handleNotification(e, inst, binding.target, part.source, part.negate); - }); + if (binding.listenerEvent) { + let part = binding.parts[0]; + node.addEventListener(binding.listenerEvent, function(e) { + handleNotification(e, inst, binding.target, part.source, part.negate); + }); + } } // -- for method-based effects (complexObserver & computed) -------------- @@ -2028,6 +2030,7 @@ // For instance-time binding, create instance of template metadata // and link into list of templates if necessary templateInfo = Object.create(templateInfo); + templateInfo.wasPreBound = wasPreBound; if (!wasPreBound && this.__templateInfo) { templateInfo.nextTemplateInfo = this.__templateInfo; this.__templateInfo.previousTemplateInfo = templateInfo; @@ -2079,7 +2082,13 @@ let templateInfo = this._bindTemplate(template, true); // Add template-instance-specific data to instanced templateInfo templateInfo.nodeList = dom.nodeList; - templateInfo.childNodes = Array.from(dom.childNodes); + // Capture child nodes to allow unstamping of non-prototypical templates + if (!templateInfo.wasPreBound) { + let nodes = templateInfo.childNodes = []; + for (let n=dom.firstChild; n; n=n.nextSibling) { + nodes.push(n); + } + } dom.templateInfo = templateInfo; // Setup compound storage, 2-way listeners, and dataHost for bindings setupBindings(this, templateInfo); From fa67457cc5d3b24f9abd047eb2d94a84b2b1ff9e Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Thu, 13 Apr 2017 16:15:15 -0700 Subject: [PATCH 17/17] Eliminate rest args for better perf on stable chrome. --- lib/mixins/property-effects.html | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index f02a24b6db..c10078ae17 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -89,16 +89,16 @@ * @param {string} type Type of effect to run * @param {Object} props Bag of current property changes * @param {Object=} oldProps Bag of previous values for changed properties - * @param {boolean} hasPaths True with `props` contains one or more paths - * @param {*} var_args Additional metadata to pass to effect function + * @param {boolean=} hasPaths True with `props` contains one or more paths + * @param {*=} extraArgs Additional metadata to pass to effect function * @private */ - function runEffects(inst, effects, props, oldProps, hasPaths, ...args) { + function runEffects(inst, effects, props, oldProps, hasPaths, extraArgs) { if (effects) { let ran; let id = dedupeId++; for (let prop in props) { - if (runEffectsForProperty(inst, effects, id, prop, props, oldProps, hasPaths, ...args)) { + if (runEffectsForProperty(inst, effects, id, prop, props, oldProps, hasPaths, extraArgs)) { ran = true; } } @@ -115,11 +115,11 @@ * @param {string} prop Name of changed property * @param {*} props Changed properties * @param {*} oldProps Old properties - * @param {boolean} hasPaths True with `props` contains one or more paths - * @param {*} var_args Additional metadata to pass to effect function + * @param {boolean=} hasPaths True with `props` contains one or more paths + * @param {*=} extraArgs Additional metadata to pass to effect function * @private */ - function runEffectsForProperty(inst, effects, dedupeId, prop, props, oldProps, hasPaths, ...args) { + function runEffectsForProperty(inst, effects, dedupeId, prop, props, oldProps, hasPaths, extraArgs) { let ran; let rootProperty = hasPaths ? Polymer.Path.root(prop) : prop; let fxs = effects[rootProperty]; @@ -130,7 +130,7 @@ if (fx.info) { fx.info.lastRun = dedupeId; } - fx.fn(inst, prop, props, oldProps, fx.info, hasPaths, ...args); + fx.fn(inst, prop, props, oldProps, fx.info, hasPaths, extraArgs); ran = true; } }