From e385e49b408f0371d870e8b32e6e2d28917176c7 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 21 Aug 2018 10:53:42 -0700 Subject: [PATCH 1/6] Add legacy-data-mixin as 1.x->2.x/3.x migration aide. Fixes #5262. --- lib/legacy/class.js | 10 +- lib/legacy/legacy-data-mixin.js | 112 ++++++++++++++++ lib/mixins/property-effects.js | 154 ++++++++++----------- test/runner.html | 3 +- test/unit/legacy-data.html | 229 ++++++++++++++++++++++++++++++++ 5 files changed, 427 insertions(+), 81 deletions(-) create mode 100644 lib/legacy/legacy-data-mixin.js create mode 100644 test/unit/legacy-data.html diff --git a/lib/legacy/class.js b/lib/legacy/class.js index fcd4db52c6..1d0e336573 100644 --- a/lib/legacy/class.js +++ b/lib/legacy/class.js @@ -334,16 +334,20 @@ function GenerateClassFromInfo(info, Base) { * * @param {!PolymerInit} info Object containing Polymer metadata and functions * to become class methods. + * @template T + * @param {function(T):T} mixin Optional mixin to apply to legacy base class * @return {function(new:HTMLElement)} Generated class */ -export const Class = function(info) { +export const Class = function(info, mixin) { if (!info) { console.warn(`Polymer's Class function requires \`info\` argument`); } - let klass = GenerateClassFromInfo(info, info.behaviors ? + const baseWithBehaviors = info.behaviors ? // note: mixinBehaviors ensures `LegacyElementMixin`. mixinBehaviors(info.behaviors, HTMLElement) : - LegacyElementMixin(HTMLElement)); + LegacyElementMixin(HTMLElement); + const baseWithMixin = mixin ? mixin(baseWithBehaviors) : baseWithBehaviors; + const klass = GenerateClassFromInfo(info, baseWithMixin); // decorate klass with registration info klass.is = info.is; return klass; diff --git a/lib/legacy/legacy-data-mixin.js b/lib/legacy/legacy-data-mixin.js new file mode 100644 index 0000000000..4401f9539c --- /dev/null +++ b/lib/legacy/legacy-data-mixin.js @@ -0,0 +1,112 @@ +/** +@license +Copyright (c) 2017 The Polymer Project Authors. All rights reserved. +This code may only be used under the BSD style license found at http://polymer.github.io/LICENSE.txt +The complete set of authors may be found at http://polymer.github.io/AUTHORS.txt +The complete set of contributors may be found at http://polymer.github.io/CONTRIBUTORS.txt +Code distributed by Google as part of the polymer project is also +subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt +*/ + +import { Class } from './class.js'; +import { Polymer } from '../../polymer-legacy.js'; +import { dedupingMixin } from '../utils/mixin.js'; + +const UndefinedArgumentError = class extends Error { + constructor(message) { + super(message); + this.name = this.constructor.name; + // Affordances for ensuring instanceof works after babel ES5 compilation + this.constructor = UndefinedArgumentError; + this.__proto__ = UndefinedArgumentError.prototype; + } +}; + +/** + * Mixin to selectively add back Polymer 1.x's `undefined` rules + * governing when observers & computing functions run based + * on all arguments being defined (reference https://www.polymer-project.org/1.0/docs/devguide/observers#multi-property-observers). + * + * When loaded, all legacy elements (defined with `Polymer({...})`) + * will have the mixin applied. The mixin only restores legacy data handling + * if `_legacyUndefinedCheck: true` is set on the element's prototype. + * + * This mixin is intended for use to help migration from Polymer 1.x to + * 2.x+ by allowing legacy code to work while identifying observers and + * computing functions that need undefined checks to work without + * the mixin in Polymer 2. + */ +export const LegacyDataMixin = dedupingMixin(superClass => { + + /** + * @polymer + * @mixinClass + * @implements {Polymer_LegacyDataMixin} + */ + class LegacyDataMixinClass extends superClass { + /** + * Overrides `Polyer.PropertyEffects` to add `undefined` argument + * checking to match Polymer 1.x style rules + * + * @param {!Array} args Array of argument metadata + * @param {string} path Property/path name that triggered the method effect + * @param {Object} props Bag of current property changes + * @return {Array<*>} Array of argument values + * @private + */ + _marshalArgs(args, path, props) { + const vals = super._marshalArgs(args, path, props); + if (this._legacyUndefinedCheck && args.length > 1) { + for (let i=0; i Class(info, + superClass => mixin ? + mixin(LegacyDataMixin(superClass)) : + LegacyDataMixin(superClass) +); + +console.info('LegacyDataMixin will be applied to all legacy elements.\n' + + 'Set `_legacyUndefinedCheck: true` on element class to enable.'); diff --git a/lib/mixins/property-effects.js b/lib/mixins/property-effects.js index fec85f8bf1..e43bccc562 100644 --- a/lib/mixins/property-effects.js +++ b/lib/mixins/property-effects.js @@ -11,7 +11,7 @@ subject to an additional IP rights grant found at http://polymer.github.io/PATEN import '../utils/boot.js'; import { dedupingMixin } from '../utils/mixin.js'; -import { root as root$0, isAncestor, isDescendant, get as get$0, translate, isPath as isPath$0, set as set$0, normalize } from '../utils/path.js'; +import { root, isAncestor, isDescendant, get, translate, isPath, set, normalize } from '../utils/path.js'; /* for notify, reflect */ import { camelToDashCase, dashToCamelCase } from '../utils/case-map.js'; import { PropertyAccessors } from './property-accessors.js'; @@ -143,7 +143,7 @@ function runEffects(inst, effects, props, oldProps, hasPaths, extraArgs) { */ function runEffectsForProperty(inst, effects, dedupeId, prop, props, oldProps, hasPaths, extraArgs) { let ran = false; - let rootProperty = hasPaths ? root$0(prop) : prop; + let rootProperty = hasPaths ? root(prop) : prop; let fxs = effects[rootProperty]; if (fxs) { for (let i=0, l=fxs.length, fx; (i} args Array of argument metadata - * @param {string} path Property/path name that triggered the method effect - * @param {Object} props Bag of current property changes - * @return {Array<*>} Array of argument values - * @private - */ -function marshalArgs(data, args, path, props) { - let values = []; - for (let i=0, l=args.length; i { */ _setPendingPropertyOrPath(path, value, shouldNotify, isPathNotification) { if (isPathNotification || - root$0(Array.isArray(path) ? path[0] : path) !== path) { + root(Array.isArray(path) ? path[0] : path) !== path) { // Dirty check changes being set to a path against the actual object, // since this is the entry point for paths into the system; from here // the only dirty checks are against the `__dataTemp` cache to prevent @@ -1376,8 +1326,8 @@ export const PropertyEffects = dedupingMixin(superClass => { // already dirty checked at the point of entry and the underlying // object has already been updated if (!isPathNotification) { - let old = get$0(this, path); - path = /** @type {string} */ (set$0(this, path, value)); + let old = get(this, path); + path = /** @type {string} */ (set(this, path, value)); // Use property-accessor's simpler dirty check if (!path || !super._shouldPropertyChange(path, value, old)) { return false; @@ -1465,8 +1415,8 @@ export const PropertyEffects = dedupingMixin(superClass => { * @return {boolean} Returns true if the property changed */ _setPendingProperty(property, value, shouldNotify) { - let isPath = this.__dataHasPaths && isPath$0(property); - let prevProps = isPath ? this.__dataTemp : this.__data; + let propIsPath = this.__dataHasPaths && isPath(property); + let prevProps = propIsPath ? this.__dataTemp : this.__data; if (this._shouldPropertyChange(property, value, prevProps[property])) { if (!this.__dataPending) { this.__dataPending = {}; @@ -1478,7 +1428,7 @@ export const PropertyEffects = dedupingMixin(superClass => { } // Paths are stored in temporary cache (cleared at end of turn), // which is used for dirty-checking, all others stored in __data - if (isPath) { + if (propIsPath) { this.__dataTemp[property] = value; } else { this.__data[property] = value; @@ -1486,7 +1436,7 @@ export const PropertyEffects = dedupingMixin(superClass => { // All changes go into pending property bag, passed to _propertiesChanged this.__dataPending[property] = value; // Track properties that should notify separately - if (isPath || (this[TYPES.NOTIFY] && this[TYPES.NOTIFY][property])) { + if (propIsPath || (this[TYPES.NOTIFY] && this[TYPES.NOTIFY][property])) { this.__dataToNotify = this.__dataToNotify || {}; this.__dataToNotify[property] = shouldNotify; } @@ -1802,7 +1752,7 @@ export const PropertyEffects = dedupingMixin(superClass => { */ notifySplices(path, splices) { let info = {path: ''}; - let array = /** @type {Array} */(get$0(this, path, info)); + let array = /** @type {Array} */(get(this, path, info)); notifySplices(this, array, info.path, splices); } @@ -1826,7 +1776,7 @@ export const PropertyEffects = dedupingMixin(superClass => { * @public */ get(path, root) { - return get$0(root || this, path); + return get(root || this, path); } /** @@ -1852,7 +1802,7 @@ export const PropertyEffects = dedupingMixin(superClass => { */ set(path, value, root) { if (root) { - set$0(root, path, value); + set(root, path, value); } else { if (!this[TYPES.READ_ONLY] || !this[TYPES.READ_ONLY][/** @type {string} */(path)]) { if (this._setPendingPropertyOrPath(path, value, true)) { @@ -1878,7 +1828,7 @@ export const PropertyEffects = dedupingMixin(superClass => { */ push(path, ...items) { let info = {path: ''}; - let array = /** @type {Array}*/(get$0(this, path, info)); + let array = /** @type {Array}*/(get(this, path, info)); let len = array.length; let ret = array.push(...items); if (items.length) { @@ -1902,7 +1852,7 @@ export const PropertyEffects = dedupingMixin(superClass => { */ pop(path) { let info = {path: ''}; - let array = /** @type {Array} */(get$0(this, path, info)); + let array = /** @type {Array} */(get(this, path, info)); let hadLength = Boolean(array.length); let ret = array.pop(); if (hadLength) { @@ -1930,7 +1880,7 @@ export const PropertyEffects = dedupingMixin(superClass => { */ splice(path, start, deleteCount, ...items) { let info = {path : ''}; - let array = /** @type {Array} */(get$0(this, path, info)); + let array = /** @type {Array} */(get(this, path, info)); // Normalize fancy native splice handling of crazy start values if (start < 0) { start = array.length - Math.floor(-start); @@ -1982,7 +1932,7 @@ export const PropertyEffects = dedupingMixin(superClass => { */ shift(path) { let info = {path: ''}; - let array = /** @type {Array} */(get$0(this, path, info)); + let array = /** @type {Array} */(get(this, path, info)); let hadLength = Boolean(array.length); let ret = array.shift(); if (hadLength) { @@ -2007,7 +1957,7 @@ export const PropertyEffects = dedupingMixin(superClass => { */ unshift(path, ...items) { let info = {path: ''}; - let array = /** @type {Array} */(get$0(this, path, info)); + let array = /** @type {Array} */(get(this, path, info)); let ret = array.unshift(...items); if (items.length) { notifySplice(this, array, info.path, 0, items.length, []); @@ -2034,7 +1984,7 @@ export const PropertyEffects = dedupingMixin(superClass => { if (arguments.length == 1) { // Get value if not supplied let info = {path: ''}; - value = get$0(this, path, info); + value = get(this, path, info); propPath = info.path; } else if (Array.isArray(path)) { // Normalize path if needed @@ -2173,6 +2123,56 @@ export const PropertyEffects = dedupingMixin(superClass => { createMethodEffect(this, sig, TYPES.COMPUTE, runComputedEffect, property, dynamicFn); } + /** + * Gather the argument values for a method specified in the provided array + * of argument metadata. + * + * The `path` and `value` arguments are used to fill in wildcard descriptor + * when the method is being called as a result of a path notification. + * + * @param {!Array} args Array of argument metadata + * @param {string} path Property/path name that triggered the method effect + * @param {Object} props Bag of current property changes + * @return {Array<*>} Array of argument values + * @private + */ + _marshalArgs(args, path, props) { + const data = this.__data; + let values = []; + for (let i=0, l=args.length; i { if (part.signature) { value = runMethodEffect(inst, path, props, oldProps, part.signature); } else if (path != part.source) { - value = get$0(inst, part.source); + value = get(inst, part.source); } else { - if (hasPaths && isPath$0(path)) { - value = get$0(inst, path); + if (hasPaths && isPath(path)) { + value = get(inst, path); } else { value = inst.__data[path]; } diff --git a/test/runner.html b/test/runner.html index 3f68f28e7a..13e6e66773 100644 --- a/test/runner.html +++ b/test/runner.html @@ -82,7 +82,8 @@ 'unit/dir.html', 'unit/disable-upgrade.html', 'unit/shady-unscoped-style.html', - 'unit/html-tag.html' + 'unit/html-tag.html', + 'unit/legacy-data.html' // 'unit/multi-style.html' ]; diff --git a/test/unit/legacy-data.html b/test/unit/legacy-data.html new file mode 100644 index 0000000000..be01f95109 --- /dev/null +++ b/test/unit/legacy-data.html @@ -0,0 +1,229 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 2d2320e5bdfd032da73755414470088d54eab83a Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 21 Aug 2018 11:37:51 -0700 Subject: [PATCH 2/6] Fix mixin jsdoc. --- lib/legacy/legacy-data-mixin.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/legacy/legacy-data-mixin.js b/lib/legacy/legacy-data-mixin.js index 4401f9539c..cc9aa2b55b 100644 --- a/lib/legacy/legacy-data-mixin.js +++ b/lib/legacy/legacy-data-mixin.js @@ -35,15 +35,20 @@ const UndefinedArgumentError = class extends Error { * 2.x+ by allowing legacy code to work while identifying observers and * computing functions that need undefined checks to work without * the mixin in Polymer 2. + * + * @mixinFunction + * @polymer + * @summary Mixin to selectively add back Polymer 1.x's `undefined` rules + * governing when observers & computing functions run. */ export const LegacyDataMixin = dedupingMixin(superClass => { /** - * @polymer - * @mixinClass - * @implements {Polymer_LegacyDataMixin} - */ - class LegacyDataMixinClass extends superClass { + * @constructor + * @extends {superClass} + * @unrestricted + * @private */ + class LegacyDataMixin extends superClass { /** * Overrides `Polyer.PropertyEffects` to add `undefined` argument * checking to match Polymer 1.x style rules @@ -98,7 +103,7 @@ export const LegacyDataMixin = dedupingMixin(superClass => { } - return LegacyDataMixinClass; + return LegacyDataMixin; }); From ae1b4173b0a303c4d16e82c88c1c8757cb594217 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Fri, 24 Aug 2018 11:45:33 -0700 Subject: [PATCH 3/6] Updates based on code review. Add computed tests. --- lib/legacy/legacy-data-mixin.js | 73 ++++++++++---- test/unit/legacy-data.html | 164 +++++++++++++++++++++++++++++++- 2 files changed, 212 insertions(+), 25 deletions(-) diff --git a/lib/legacy/legacy-data-mixin.js b/lib/legacy/legacy-data-mixin.js index cc9aa2b55b..018ccd31f9 100644 --- a/lib/legacy/legacy-data-mixin.js +++ b/lib/legacy/legacy-data-mixin.js @@ -13,15 +13,43 @@ import { Polymer } from '../../polymer-legacy.js'; import { dedupingMixin } from '../utils/mixin.js'; const UndefinedArgumentError = class extends Error { - constructor(message) { + constructor(message, arg) { super(message); + this.arg = arg; this.name = this.constructor.name; // Affordances for ensuring instanceof works after babel ES5 compilation + // TODO(kschaaf): Remove after polymer CLI updates to newer Babel that + // sets the constructor/prototype correctly for subclassed builtins this.constructor = UndefinedArgumentError; this.__proto__ = UndefinedArgumentError.prototype; } }; +/** + * Wraps effect functions to catch `UndefinedArgumentError`s and warn. + * + * @param {Object=} effect Effect metadata object + * @param {Object=} fnName Name of user function, if known + * @return {Object=} Effect metadata object + */ +function wrapEffect(effect, fnName) { + if (effect && effect.fn) { + const fn = effect.fn; + effect.fn = function() { + try { + fn.apply(this, arguments); + } catch (e) { + if (e instanceof UndefinedArgumentError) { + console.warn(`Argument '${e.arg}'${fnName ?` for method '${fnName}'` : ''} was undefined. Ensure it has an undefined check.`); + } else { + throw e; + } + } + }; + } + return effect; +} + /** * Mixin to selectively add back Polymer 1.x's `undefined` rules * governing when observers & computing functions run based @@ -50,7 +78,7 @@ export const LegacyDataMixin = dedupingMixin(superClass => { * @private */ class LegacyDataMixin extends superClass { /** - * Overrides `Polyer.PropertyEffects` to add `undefined` argument + * Overrides `Polymer.PropertyEffects` to add `undefined` argument * checking to match Polymer 1.x style rules * * @param {!Array} args Array of argument metadata @@ -61,12 +89,16 @@ export const LegacyDataMixin = dedupingMixin(superClass => { */ _marshalArgs(args, path, props) { const vals = super._marshalArgs(args, path, props); - if (this._legacyUndefinedCheck && args.length > 1) { - for (let i=0; i 1) { + for (let i=0; i { * @protected */ _addPropertyEffect(property, type, effect) { - if (effect && effect.fn) { - const fn = effect.fn; - effect.fn = function() { - try { - fn.apply(this, arguments); - } catch (e) { - if (e instanceof UndefinedArgumentError) { - console.warn(e.message); - } else { - throw e; - } - } - }; - } - return super._addPropertyEffect(property, type, effect); + return super._addPropertyEffect(property, type, + wrapEffect(effect, effect && effect.info && effect.info.methodName)); + } + + /** + * Overrides `Polyer.PropertyEffects` to wrap effect functions to + * catch `UndefinedArgumentError`s and warn. + * + * @param {Object} templateInfo Template metadata to add effect to + * @param {string} prop Property that should trigger the effect + * @param {Object=} effect Effect metadata object + * @return {void} + * @protected + */ + static _addTemplatePropertyEffect(templateInfo, prop, effect) { + return super._addTemplatePropertyEffect(templateInfo, prop, wrapEffect(effect)); } } diff --git a/test/unit/legacy-data.html b/test/unit/legacy-data.html index be01f95109..3747c9f561 100644 --- a/test/unit/legacy-data.html +++ b/test/unit/legacy-data.html @@ -20,16 +20,34 @@ +