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

[2.x] Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262. #5266

Merged
merged 22 commits into from
Nov 2, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c46a38d
Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262.
kevinpschaaf Jun 21, 2018
4ec63a4
Add legacy-data-mixin tests to runner.
kevinpschaaf Jun 21, 2018
2214b9e
Ensure class.html is imported before patching it.
kevinpschaaf Jun 21, 2018
7fc9564
Update types.
kevinpschaaf Jun 21, 2018
b3bbd21
Fix method to force CE polyfill on
kevinpschaaf Jun 21, 2018
a470e45
Fix test name
kevinpschaaf Jun 21, 2018
6818554
Add constructor name and fix mixin wrapping.
kevinpschaaf Jun 22, 2018
08e13f2
Factor out some helpers, add declarative tests.
kevinpschaaf Jun 22, 2018
c550e9e
Add comment
kevinpschaaf Jun 22, 2018
61d1143
Merge branch '2.x' into 5262-kschaaf-legacy-undefined
dfreedm Jun 28, 2018
b856b89
Merge branch '2.x' into 5262-kschaaf-legacy-undefined
kevinpschaaf Jul 18, 2018
5992774
Fix update-types.
kevinpschaaf Jul 18, 2018
d44969a
Ensure instanceof works after babel ES5 compilation
kevinpschaaf Jul 23, 2018
ed6deea
Sync with changes made on master.
kevinpschaaf Oct 31, 2018
763e40d
Merge branch '2.x' into 5262-kschaaf-legacy-undefined
kevinpschaaf Oct 31, 2018
7f2fcb1
Update types.
kevinpschaaf Oct 31, 2018
55e9dfd
Apply mixin to TemplatizeInstance
kevinpschaaf Oct 31, 2018
509b73f
Update wct for FF63, add FF62 to sauce for pre-WC testing, drop Safar…
kevinpschaaf Oct 31, 2018
75cfcb8
Update types.
kevinpschaaf Oct 31, 2018
9f65049
Use windows for FF62
kevinpschaaf Oct 31, 2018
6e4f62d
Add comment and handle undefined host case.
kevinpschaaf Oct 31, 2018
d91ac93
Disable running FF in headless mode on Travis
kevinpschaaf Oct 31, 2018
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
10 changes: 7 additions & 3 deletions lib/legacy/class.html
Original file line number Diff line number Diff line change
Expand Up @@ -353,17 +353,21 @@
*
* @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
* @memberof Polymer
*/
Polymer.Class = function(info) {
Polymer.Class = function(info, mixin) {
if (!info) {
console.warn('Polymer.Class requires `info` argument');
}
let klass = GenerateClassFromInfo(info, info.behaviors ?
const baseWithBehaviors = info.behaviors ?
// note: mixinBehaviors ensures `LegacyElementMixin`.
mixinBehaviors(info.behaviors, HTMLElement) :
Polymer.LegacyElementMixin(HTMLElement));
Polymer.LegacyElementMixin(HTMLElement);
const baseWithMixin = mixin ? mixin(baseWithBehaviors) : baseWithBehaviors;
const klass = GenerateClassFromInfo(info, baseWithMixin);
// decorate klass with registration info
klass.is = info.is;
return klass;
Expand Down
105 changes: 105 additions & 0 deletions lib/legacy/legacy-data-mixin.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
<!--
@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
-->

<link rel="import" href="../class.html">
<link rel="import" href="../utils/mixin.html">

<script>
(function() {
'use strict';

const UndefinedArgumentError = class extends Error {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want this error to have a meanifulful name this pattern should be followed https://azimi.me/2015/09/23/high-custom-error-class-es6.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


/**
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why we are _ prefixing this prototype value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a convention, we generally underscore any property not intended as part of the DOM element's public API, and consider them "protected". https://www.polymer-project.org/2.0/docs/devguide/properties#private-and-protected-properties

*
* 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.
*/
Polymer.LegacyDataMixin = Polymer.dedupingMixin(superClass => {

/**
* @polymer
* @mixinClass
* @implements {Polymer_LegacyDataMixin}
*/
class LegacyDataMixin extends superClass {
/**
* Overrides `Polyer.PropertyEffects` to add `undefined` argument
* checking to match Polymer 1.x style rules
*
* @param {!Array<!MethodArg>} 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<args.length; i++) {
if (vals[i] === undefined) {
throw new UndefinedArgumentError(`argument '${args[i].name}' is undefined; ensure it has an undefined check`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we are throwing an error here to break out of the control flow, as code later executed would erroneously run? If that is correct, maybe add a comment to make this explicit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}
}
return vals;
}

/**
* Overrides `Polyer.PropertyEffects` to wrap effect functions to
* catch `UndefinedArgumentError`s and warn.
*
* @param {string} property Property that should trigger the effect
* @param {string} type Effect type, from this.PROPERTY_EFFECT_TYPES
* @param {Object=} effect Effect metadata object
* @return {void}
* @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 LegacyDataMixin;

});

const Class = Polymer.Class;
Polymer.Class = info => Class(info, Polymer.LegacyDataMixin);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break if a mixin was passed in to this function. Therefore, I think the mixin should be optional and the mixin should be conditionally applied to this mixin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch. The mixin option was a late addition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch. Updated.


console.info('LegacyDataMixin will be applied to all legacy elements.\n' +
'Set `_legacyUndefinedCheck: true` to enable.');

})();
</script>
102 changes: 51 additions & 51 deletions lib/mixins/property-effects.html
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@
let context = inst._methodHost || inst;
let fn = context[info.methodName];
if (fn) {
let args = marshalArgs(inst.__data, info.args, property, props);
let args = inst._marshalArgs(info.args, property, props);
return fn.apply(context, args);
} else if (!info.dynamicFn) {
console.warn('method `' + info.methodName + '` not defined');
Expand Down Expand Up @@ -970,56 +970,6 @@
return a;
}

/**
* 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 {Object} data Instance data storage object to read properties from
* @param {!Array<!MethodArg>} 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<l; i++) {
let arg = args[i];
let name = arg.name;
let v;
if (arg.literal) {
v = arg.value;
} else {
if (arg.structured) {
v = Polymer.Path.get(data, name);
// when data is not stored e.g. `splices`
if (v === undefined) {
v = props[name];
}
} else {
v = data[name];
}
}
if (arg.wildcard) {
// Only send the actual path changed info if the change that
// caused the observer to run matched the wildcard
let baseChanged = (name.indexOf(path + '.') === 0);
let matches = (path.indexOf(name) === 0 && !baseChanged);
values[i] = {
path: matches ? path : name,
value: matches ? props[path] : v,
base: v
};
} else {
values[i] = v;
}
}
return values;
}

// data api

/**
Expand Down Expand Up @@ -2180,6 +2130,56 @@
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<!MethodArg>} args Array of argument metadata
* @param {string} path Property/path name that triggered the method effect
* @param {Object} props Bag of current property changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is a bag? perhaps restate as a "hash" or key value collection of ...

* @return {Array<*>} Array of argument values
* @private
*/
_marshalArgs(args, path, props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function has a non-trivial cyclical complexity score, can you please add unit tests to ensure the functionality is correct. Also it might be a sign that the logic should be split up into smaller more meaningful functions to maintain a higher degree of readability.

const data = this.__data;
let values = [];
for (let i=0, l=args.length; i<l; i++) {
let arg = args[i];
let name = arg.name;
let v;
if (arg.literal) {
v = arg.value;
} else {
if (arg.structured) {
v = Polymer.Path.get(data, name);
// when data is not stored e.g. `splices`
if (v === undefined) {
v = props[name];
}
} else {
v = data[name];
}
}
if (arg.wildcard) {
// Only send the actual path changed info if the change that
// caused the observer to run matched the wildcard
let baseChanged = (name.indexOf(path + '.') === 0);
let matches = (path.indexOf(name) === 0 && !baseChanged);
values[i] = {
path: matches ? path : name,
value: matches ? props[path] : v,
base: v
};
} else {
values[i] = v;
}
}
return values;
}

// -- static class methods ------------

/**
Expand Down
3 changes: 2 additions & 1 deletion test/runner.html
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,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'
];

Expand Down
Loading