Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid over-warning on templatizer props and "static" dynamicFns. #5494

Merged
merged 7 commits into from
Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -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;
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,7 +26,8 @@
<script type="module">
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() {
Expand Down Expand Up @@ -1881,23 +1882,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