From c966eb1fa63c225f7b8f7d90e6e3e22a9b1f7594 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 19 Feb 2019 12:45:34 -0800 Subject: [PATCH 1/7] Avoid over-warning on templatizer props and "static" dynamicFns. --- lib/mixins/element-mixin.js | 5 ++++- lib/mixins/property-effects.js | 2 +- lib/mixins/template-stamp.js | 1 + test/unit/property-effects.html | 24 ++++++++++++++++++++++++ 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/lib/mixins/element-mixin.js b/lib/mixins/element-mixin.js index c931fb7fc0..415e2e2b3d 100644 --- a/lib/mixins/element-mixin.js +++ b/lib/mixins/element-mixin.js @@ -777,7 +777,10 @@ export const ElementMixin = dedupingMixin(base => { // The warning is only enabled in `legacyOptimizations` mode, since // we don't want to spam existing users who might have adopted the // shorthand when attribute deserialization is not important. - if (legacyOptimizations && !(prop in this._properties)) { + if (legacyOptimizations && !(prop in this._properties) && + effect.info && effect.info.part && + !(effect.info.part.signature && effect.info.part.signature.static) && + !effect.info.part.hostProp && !templateInfo.nestedTemplate) { console.warn(`Property '${prop}' used in template but not declared in 'properties'; ` + `attribute will not be observed.`); } diff --git a/lib/mixins/property-effects.js b/lib/mixins/property-effects.js index c92bea3fa6..65d76136be 100644 --- a/lib/mixins/property-effects.js +++ b/lib/mixins/property-effects.js @@ -2640,7 +2640,7 @@ export const PropertyEffects = dedupingMixin(superClass => { let hostProps = nodeInfo.templateInfo.hostProps; let mode = '{'; for (let source in hostProps) { - let parts = [{ mode, source, dependencies: [source] }]; + let parts = [{ mode, source, dependencies: [source], hostProp: true }]; addBinding(this, templateInfo, nodeInfo, 'property', '_host_' + source, parts); } return noted; diff --git a/lib/mixins/template-stamp.js b/lib/mixins/template-stamp.js index 167fbe530a..156bacee5f 100644 --- a/lib/mixins/template-stamp.js +++ b/lib/mixins/template-stamp.js @@ -204,6 +204,7 @@ export const TemplateStamp = dedupingMixin( if (!template._templateInfo) { let templateInfo = template._templateInfo = {}; templateInfo.nodeInfoList = []; + templateInfo.nestedTemplate = Boolean(outerTemplateInfo); templateInfo.stripWhiteSpace = (outerTemplateInfo && outerTemplateInfo.stripWhiteSpace) || template.hasAttribute('strip-whitespace'); diff --git a/test/unit/property-effects.html b/test/unit/property-effects.html index 087023914f..61a2a75083 100644 --- a/test/unit/property-effects.html +++ b/test/unit/property-effects.html @@ -1900,6 +1900,30 @@ assert.equal(console.warn.callCount, 3); }); + test('do not warn on valid non-property bindings', () => { + setLegacyOptimizations(true); + Polymer({ + is: 'x-nowarn-undeclared-binding', + properties: { + hostProp: String + }, + _template: html` +
[[noDeps()]]
+
[[hostProp]]
+ + `, + noDeps() { return 'static'; } + }); + const el = document.createElement('x-nowarn-undeclared-binding'); + document.body.appendChild(el); + document.body.removeChild(el); + assert.equal(console.warn.callCount, 0); + }); + test('warn when re-declaring a computed property', () => { Polymer({ is: 'x-warn-redeclared-computed', From 2874c86d8d85a2bf22d16340eb15d7ee79c75d71 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 19 Feb 2019 12:48:17 -0800 Subject: [PATCH 2/7] Remove unnecessary qualification. --- lib/mixins/element-mixin.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/mixins/element-mixin.js b/lib/mixins/element-mixin.js index 415e2e2b3d..976f18c755 100644 --- a/lib/mixins/element-mixin.js +++ b/lib/mixins/element-mixin.js @@ -778,7 +778,6 @@ export const ElementMixin = dedupingMixin(base => { // we don't want to spam existing users who might have adopted the // shorthand when attribute deserialization is not important. if (legacyOptimizations && !(prop in this._properties) && - effect.info && effect.info.part && !(effect.info.part.signature && effect.info.part.signature.static) && !effect.info.part.hostProp && !templateInfo.nestedTemplate) { console.warn(`Property '${prop}' used in template but not declared in 'properties'; ` + From 940d1a7a0ed3b5967f750e4dbcd36747ea8fef14 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 20 Feb 2019 01:22:24 -0800 Subject: [PATCH 3/7] Add comments on warning limitations. --- lib/mixins/element-mixin.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/mixins/element-mixin.js b/lib/mixins/element-mixin.js index 976f18c755..7aeca81e92 100644 --- a/lib/mixins/element-mixin.js +++ b/lib/mixins/element-mixin.js @@ -778,8 +778,18 @@ export const ElementMixin = dedupingMixin(base => { // we don't want to spam existing users who might have adopted the // shorthand when attribute deserialization is not important. if (legacyOptimizations && !(prop in this._properties) && + // Methods used in templates with no dependencies (or only literal + // dependencies) become accessors with template effects; ignore these !(effect.info.part.signature && effect.info.part.signature.static) && - !effect.info.part.hostProp && !templateInfo.nestedTemplate) { + // Bindings added from a host to the template element may also include + // "instance props" introduced into the template scope and not + // actually provided by the host; since we can't disambiguate these, + // ignore them all + !effect.info.part.hostProp && + // Template effects added on TemplateInstance classes can't determine + // whether the property may have been a "static" method, so ignore + // all nested template bindings + !templateInfo.nestedTemplate) { console.warn(`Property '${prop}' used in template but not declared in 'properties'; ` + `attribute will not be observed.`); } From aa63db00439f3050df85fafb26a37ae7b137a478 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 20 Feb 2019 17:32:33 -0800 Subject: [PATCH 4/7] Add templatizer warnings. Move to legacyWarnings flag. --- lib/mixins/element-mixin.js | 18 +++++-------- lib/utils/settings.js | 15 +++++++++++ lib/utils/templatize.js | 36 +++++++++++++++++++++++--- test/unit/property-effects.html | 45 ++++++++++++++++++++++++--------- 4 files changed, 87 insertions(+), 27 deletions(-) diff --git a/lib/mixins/element-mixin.js b/lib/mixins/element-mixin.js index 7aeca81e92..a510ecc9cc 100644 --- a/lib/mixins/element-mixin.js +++ b/lib/mixins/element-mixin.js @@ -11,7 +11,7 @@ */ import '../utils/boot.js'; -import { rootPath, strictTemplatePolicy, allowTemplateFromDomModule, legacyOptimizations, syncInitialRender } from '../utils/settings.js'; +import { rootPath, strictTemplatePolicy, allowTemplateFromDomModule, legacyOptimizations, legacyWarnings, syncInitialRender} from '../utils/settings.js'; import { dedupingMixin } from '../utils/mixin.js'; import { stylesFromTemplate, stylesFromModuleImports } from '../utils/style-gather.js'; import { pathFromUrl, resolveCss, resolveUrl } from '../utils/resolve-url.js'; @@ -777,19 +777,15 @@ export const ElementMixin = dedupingMixin(base => { // The warning is only enabled in `legacyOptimizations` mode, since // we don't want to spam existing users who might have adopted the // shorthand when attribute deserialization is not important. - if (legacyOptimizations && !(prop in this._properties) && + if (legacyWarnings && !(prop in this._properties) && // Methods used in templates with no dependencies (or only literal // dependencies) become accessors with template effects; ignore these !(effect.info.part.signature && effect.info.part.signature.static) && - // Bindings added from a host to the template element may also include - // "instance props" introduced into the template scope and not - // actually provided by the host; since we can't disambiguate these, - // ignore them all - !effect.info.part.hostProp && - // Template effects added on TemplateInstance classes can't determine - // whether the property may have been a "static" method, so ignore - // all nested template bindings - !templateInfo.nestedTemplate) { + // Warnings for bindings added to nested templates are handled by + // templatizer so ignore both the host-to-template bindings + // (`hostProp`) and TemplateInstance-to-child bindings + // (`nestedTemplate`) + !effect.info.part.hostProp && !templateInfo.nestedTemplate) { console.warn(`Property '${prop}' used in template but not declared in 'properties'; ` + `attribute will not be observed.`); } diff --git a/lib/utils/settings.js b/lib/utils/settings.js index 6b878c4ed1..d429f597d5 100644 --- a/lib/utils/settings.js +++ b/lib/utils/settings.js @@ -141,6 +141,21 @@ export const setLegacyOptimizations = function(useLegacyOptimizations) { legacyOptimizations = useLegacyOptimizations; }; +/** + * Setting to add warnings useful when migrating from Polymer 1.x to 2.x. + */ +export let legacyWarnings = false; + +/** + * Sets `legacyWarnings` globally for all elements to migration warnings. + * + * @param {boolean} useLegacyWarnings enable or disable warnings + * @return {void} + */ +export const setLegacyWarnings = function(useLegacyWarnings) { + legacyWarnings = useLegacyWarnings; +}; + /** * Setting to perform initial rendering synchronously when running under ShadyDOM. * This matches the behavior of Polymer 1. diff --git a/lib/utils/templatize.js b/lib/utils/templatize.js index 9f05135470..a85852f174 100644 --- a/lib/utils/templatize.js +++ b/lib/utils/templatize.js @@ -49,7 +49,7 @@ import './boot.js'; import { PropertyEffects } from '../mixins/property-effects.js'; import { MutableData } from '../mixins/mutable-data.js'; -import { strictTemplatePolicy } from './settings.js'; +import { strictTemplatePolicy, legacyWarnings } from './settings.js'; import { wrap } from './wrap.js'; // Base class for HTMLTemplateElement extension that has property effects @@ -362,7 +362,7 @@ function createTemplatizerClass(template, templateInfo, options) { /** * @suppress {missingProperties} class.prototype is not defined for some reason */ -function addPropagateEffects(template, templateInfo, options) { +function addPropagateEffects(template, templateInfo, options, methodHost) { let userForwardHostProp = options.forwardHostProp; if (userForwardHostProp) { // Provide data API and property effects on memoized template class @@ -385,6 +385,9 @@ function addPropagateEffects(template, templateInfo, options) { {fn: createForwardHostPropEffect(prop, userForwardHostProp)}); klass.prototype._createNotifyingProperty('_host_' + prop); } + if (legacyWarnings && methodHost) { + warnOnUndeclaredProperties(templateInfo, options, methodHost); + } } upgradeTemplate(template, klass); // Mix any pre-bound data into __data; no need to flush this to @@ -547,13 +550,14 @@ export function templatize(template, owner, options) { baseClass = createTemplatizerClass(template, templateInfo, options); templateInfo.templatizeInstanceClass = baseClass; } + const methodHost = findMethodHost(template) // Host property forwarding must be installed onto template instance - addPropagateEffects(template, templateInfo, options); + addPropagateEffects(template, templateInfo, options, methodHost); // Subclass base class and add reference for this specific template /** @private */ let klass = class TemplateInstance extends baseClass {}; /** @override */ - klass.prototype._methodHost = findMethodHost(template); + klass.prototype._methodHost = methodHost; /** @override */ klass.prototype.__dataHost = /** @type {!DataTemplate} */ (template); /** @override */ @@ -564,6 +568,30 @@ export function templatize(template, owner, options) { return klass; } +function warnOnUndeclaredProperties(templateInfo, options, methodHost) { + const declaredProps = methodHost.constructor._properties; + const {propertyEffects} = templateInfo; + const {instanceProps} = options; + for (let prop in propertyEffects) { + // Ensure properties with template effects are declared on the outermost + // host (`methodHost`), unless they are instance props or static functions + if (!declaredProps[prop] && !(instanceProps && instanceProps[prop])) { + const effects = propertyEffects[prop]; + for (let i=0; i import './property-effects-elements.js'; import { Polymer, html } from '../../polymer-legacy.js'; -import { setSanitizeDOMValue, sanitizeDOMValue, setLegacyOptimizations } from '../../lib/utils/settings.js'; +import { flush } from '../../lib/utils/flush.js'; +import { setSanitizeDOMValue, sanitizeDOMValue, setLegacyWarnings } from '../../lib/utils/settings.js'; import { PropertyEffects } from '../../lib/mixins/property-effects.js'; suite('single-element binding effects', function() { @@ -1881,45 +1882,65 @@ }); teardown(function() { - setLegacyOptimizations(false); + setLegacyWarnings(false); console.warn.restore(); }); test('warn if non-declared property used in binding', () => { - setLegacyOptimizations(true); + setLegacyWarnings(true); Polymer({ is: 'x-warn-undeclared-binding', _template: html`
- [[undeclaredToText]]
` + [[undeclaredToText]] + [[compute(undeclaredDep1, undeclaredDep2)]] + + + `, + compute(dep) { return dep; } }); const el = document.createElement('x-warn-undeclared-binding'); document.body.appendChild(el); + flush(); document.body.removeChild(el); - assert.equal(console.warn.callCount, 3); + assert.equal(console.warn.callCount, 10); }); test('do not warn on valid non-property bindings', () => { - setLegacyOptimizations(true); + setLegacyWarnings(true); Polymer({ is: 'x-nowarn-undeclared-binding', properties: { - hostProp: String + hostProp: String, + dynamicFn: Function }, _template: html` -
[[noDeps()]]
-
[[hostProp]]
+
[[hostProp]]
+
[[noDeps()]]
+
[[dynamicFn()]]
+
[[dynamicFnWithDep(hostProp)]]
`, - noDeps() { return 'static'; } + noDeps() { return 'static'; }, + dynamicFn(dep) { return dep; }, + dynamicFnWithDep(dep) { return dep; } }); const el = document.createElement('x-nowarn-undeclared-binding'); document.body.appendChild(el); + flush(); document.body.removeChild(el); assert.equal(console.warn.callCount, 0); }); From 11bdc39a6d22b19123b9d18ff71487c6af72dc8f Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 20 Feb 2019 17:45:34 -0800 Subject: [PATCH 5/7] Simplify --- lib/utils/templatize.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/utils/templatize.js b/lib/utils/templatize.js index a85852f174..c53abdab6f 100644 --- a/lib/utils/templatize.js +++ b/lib/utils/templatize.js @@ -578,14 +578,11 @@ function warnOnUndeclaredProperties(templateInfo, options, methodHost) { if (!declaredProps[prop] && !(instanceProps && instanceProps[prop])) { const effects = propertyEffects[prop]; for (let i=0; i Date: Wed, 20 Feb 2019 17:54:14 -0800 Subject: [PATCH 6/7] Remove unnecessary literal check --- lib/utils/templatize.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/utils/templatize.js b/lib/utils/templatize.js index c53abdab6f..bce85db55b 100644 --- a/lib/utils/templatize.js +++ b/lib/utils/templatize.js @@ -579,7 +579,7 @@ function warnOnUndeclaredProperties(templateInfo, options, methodHost) { const effects = propertyEffects[prop]; for (let i=0; i Date: Wed, 20 Feb 2019 17:57:36 -0800 Subject: [PATCH 7/7] Add test for literal-only static function. --- test/unit/property-effects.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/property-effects.html b/test/unit/property-effects.html index b4f619db56..dbc2a43909 100644 --- a/test/unit/property-effects.html +++ b/test/unit/property-effects.html @@ -1924,12 +1924,14 @@ _template: html`
[[hostProp]]
[[noDeps()]]
+
[[literalDeps('hi')]]
[[dynamicFn()]]
[[dynamicFnWithDep(hostProp)]]