Skip to content

Commit

Permalink
Merge branch '5493-kschaaf-fix-warnings' into legacy-undefined-noBatch
Browse files Browse the repository at this point in the history
  • Loading branch information
kevinpschaaf committed Feb 21, 2019
2 parents bef674a + 4c65db8 commit a9cd534
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 12 deletions.
12 changes: 10 additions & 2 deletions lib/mixins/element-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -777,7 +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) &&
// 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.`);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/mixins/property-effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -2713,7 +2713,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;
Expand Down
1 change: 1 addition & 0 deletions lib/mixins/template-stamp.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
15 changes: 15 additions & 0 deletions lib/utils/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
33 changes: 29 additions & 4 deletions lib/utils/templatize.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 */
Expand All @@ -564,6 +568,27 @@ 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<effects.length; i++) {
const {part} = effects[i].info;
if (!(part.signature && part.signature.static)) {
console.warn(`Property '${prop}' used in template but not ` +
`declared in 'properties'; attribute will not be observed.`);
break;
}
}
}
}
}

/**
* Returns the template "model" associated with a given element, which
* serves as the binding scope for the template instance the element is
Expand Down
57 changes: 52 additions & 5 deletions test/unit/property-effects.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@
<script type="module">
import './property-effects-elements.js';
import { Polymer, html } from '../../polymer-legacy.js';
import { setSanitizeDOMValue, sanitizeDOMValue, setLegacyOptimizations, legacyUndefined, setLegacyUndefined, legacyNoBatch, setLegacyNoBatch, legacyNotifyOrder, setLegacyNotifyOrder } from '../../lib/utils/settings.js';
import { setSanitizeDOMValue, sanitizeDOMValue, setLegacyWarnings, setLegacyOptimizations, legacyUndefined, setLegacyUndefined, legacyNoBatch, setLegacyNoBatch, legacyNotifyOrder, setLegacyNotifyOrder } from '../../lib/utils/settings.js';
import { PropertyEffects } from '../../lib/mixins/property-effects.js';
import { flush } from '../../lib/utils/flush.js';

setLegacyUndefined(Boolean(window.location.search.match('legacyUndefined')));
setLegacyNoBatch(Boolean(window.location.search.match('legacyNoBatch')));
Expand Down Expand Up @@ -1944,23 +1945,69 @@
});

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`
<div attr$="[[undeclaredToAttr]]"
prop="[[undeclaredToProp]]">
[[undeclaredToText]]</div>`
[[undeclaredToText]]
[[compute(undeclaredDep1, undeclaredDep2)]]
</div>
<template is="dom-repeat" items="[0]">
<div attr$="[[nestedUndeclaredToAttr]]"
prop="[[nestedUndeclaredToProp]]">
[[nestedUndeclaredToText]]
[[compute(nestedUndeclaredDep1, nestedUndeclaredDep2)]]
</div>
</template>
`,
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', () => {
setLegacyWarnings(true);
Polymer({
is: 'x-nowarn-undeclared-binding',
properties: {
hostProp: String,
dynamicFn: Function
},
_template: html`
<div>[[hostProp]]</div> <!-- host prop -->
<div>[[noDeps()]]</div> <!-- method with no deps -->
<div>[[literalDeps('hi')]]</div> <!-- method with literal deps -->
<div>[[dynamicFn()]]</div> <!-- dynamic function with no deps -->
<div>[[dynamicFnWithDep(hostProp)]]</div> <!-- dynamic function with deps -->
<template is="dom-repeat" items="[0]" as="instProp">
<div>[[instProp]]</div> <!-- instance prop -->
<div>[[hostProp]]</div> <!-- nested host prop -->
<div>[[noDeps()]]</div> <!-- nested method with no deps -->
<div>[[literalDeps('hi')]]</div> <!-- method with literal deps -->
<div>[[dynamicFn()]]</div> <!-- dynamic function with no deps -->
<div>[[dynamicFnWithDep(hostProp)]]</div> <!-- dynamic function with deps -->
</template>
`,
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);
});

test('warn when re-declaring a computed property', () => {
Expand Down

0 comments on commit a9cd534

Please sign in to comment.