From aa63db00439f3050df85fafb26a37ae7b137a478 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 20 Feb 2019 17:32:33 -0800 Subject: [PATCH] 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); });