Skip to content

Commit

Permalink
Updates based on code review. Add computed tests.
Browse files Browse the repository at this point in the history
  • Loading branch information
@kevinpschaaf
kevinpschaaf committed Aug 24, 2018
1 parent 2d2320e commit ae1b417
Show file tree
Hide file tree
Showing 2 changed files with 212 additions and 25 deletions.
73 changes: 53 additions & 20 deletions lib/legacy/legacy-data-mixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<!MethodArg>} args Array of argument metadata
Expand All @@ -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<args.length; i++) {
// Per legacy data rules, single-property observers (whether in `properties`
// and in `observers`) are called regardless of whether their argument is
// undefined or not. Multi-property observers must have all arguments defined
if (this._legacyUndefinedCheck && vals.length > 1) {
for (let i=0; i<vals.length; i++) {
if (vals[i] === undefined) {
// Break out of effect's control flow; will be caught in
// wrapped property effect function below
throw new UndefinedArgumentError(`argument '${args[i].name}' is undefined; ensure it has an undefined check`);
const name = args[i].name;
throw new UndefinedArgumentError(`Argument '${name}' is undefined. Ensure it has an undefined check.`, name);
}
}
}
Expand All @@ -84,21 +116,22 @@ export const LegacyDataMixin = dedupingMixin(superClass => {
* @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));
}

}
Expand Down
164 changes: 159 additions & 5 deletions test/unit/legacy-data.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,34 @@
<body>

<dom-module id="x-data">
<template>
<div id="child"
computed-single="[[computeSingle(inlineSingleDep)]]"
computed-multi="[[computeMulti(inlineMultiDep1, inlineMultiDep2)]]">
</div>
</template>
<script type="module">
import {Polymer} from '../../polymer-legacy.js';
Polymer({
is: 'x-data',
_legacyUndefinedCheck: true,
properties: {
singleProp: String,
multiProp1: String,
multiProp2: String
multiProp2: String,
computedSingleDep: String,
computedMultiDep1: String,
computedMultiDep2: String,
inlineSingleDep: String,
inlineMultiDep1: String,
inlineMultiDep2: String,
computedSingle: {
computed: 'computeSingle(computedSingleDep)'
},
computedMulti: {
computed: 'computeMulti(computedMultiDep1, computedMultiDep2)'
}
},
_legacyUndefinedCheck: true,
observers: [
'staticObserver("staticObserver")',
'singlePropObserver(singleProp)',
Expand All @@ -40,6 +58,8 @@
this.singlePropObserver = sinon.spy();
this.multiPropObserver = sinon.spy();
this.staticObserver = sinon.spy();
this.computeSingle = sinon.spy((inlineSingleDep) => `[${inlineSingleDep}]`);
this.computeMulti = sinon.spy((inlineMultiDep1, inlineMultiDep2) => `[${inlineMultiDep1},${inlineMultiDep2}]`);
},
throws() {
throw new Error('real error');
Expand Down Expand Up @@ -72,6 +92,42 @@
</template>
</test-fixture>

<test-fixture id="declarative-single-computed">
<template>
<x-data computed-single-dep="a"></x-data>
</template>
</test-fixture>

<test-fixture id="declarative-multi-one-computed">
<template>
<x-data computed-multi-dep1="b"></x-data>
</template>
</test-fixture>

<test-fixture id="declarative-multi-all-computed">
<template>
<x-data computed-multi-dep1="b" computed-multi-dep2="c"></x-data>
</template>
</test-fixture>

<test-fixture id="declarative-single-computed-inline">
<template>
<x-data inline-single-dep="a"></x-data>
</template>
</test-fixture>

<test-fixture id="declarative-multi-one-computed-inline">
<template>
<x-data inline-multi-dep1="b"></x-data>
</template>
</test-fixture>

<test-fixture id="declarative-multi-all-computed-inline">
<template>
<x-data inline-multi-dep1="b" inline-multi-dep2="c"></x-data>
</template>
</test-fixture>

<script>
(function() {

Expand All @@ -83,6 +139,10 @@
callCounts.singlePropObserver || 0, 'singlePropObserver call count wrong');
assert.equal(el.multiPropObserver.callCount,
callCounts.multiPropObserver || 0, 'multiPropObserver call count wrong');
assert.equal(el.computeSingle.callCount,
callCounts.computeSingle || 0, 'computeSingle call count wrong');
assert.equal(el.computeMulti.callCount,
callCounts.computeMulti || 0, 'computeMulti call count wrong');
assert.equal(console.warn.callCount, callCounts.warn || 0,
'console.warn call count wrong');
}
Expand All @@ -103,9 +163,15 @@
el.parentNode.removeChild(el);
});

const singleProp = 'a';
const multiProp1 = 'b';
const multiProp2 = 'c';
const singleProp = 'singleProp';
const multiProp1 = 'multiProp1';
const multiProp2 = 'multiProp2';
const computedSingleDep = 'computedSingleDep';
const computedMultiDep1 = 'computedMultiDep1';
const computedMultiDep2 = 'computedMultiDep2';
const inlineSingleDep = 'inlineSingleDep';
const inlineMultiDep1 = 'inlineMultiDep1';
const inlineMultiDep2 = 'inlineMultiDep2';

suite('check disabled', () => {
test('no arguments defined', () => {
Expand Down Expand Up @@ -144,6 +210,36 @@
el.multiProp2 = undefined;
assertEffects({multiPropObserver: 3});
});
test('computeSingle argument defined', () => {
setupElement(false, {computedSingleDep});
assertEffects({computeSingle: 1});
assert.equal(el.computedSingle, '[computedSingleDep]');
});
test('one computeMulti argument defined', () => {
setupElement(false, {computedMultiDep1});
assertEffects({computeMulti: 1});
assert.equal(el.computedMulti, '[computedMultiDep1,undefined]');
});
test('all computeMulti argument defined', () => {
setupElement(false, {computedMultiDep1, computedMultiDep2});
assertEffects({computeMulti: 1});
assert.equal(el.computedMulti, '[computedMultiDep1,computedMultiDep2]');
});
test('inline computeSingle argument defined', () => {
setupElement(false, {inlineSingleDep});
assertEffects({computeSingle: 1});
assert.equal(el.$.child.computedSingle, '[inlineSingleDep]');
});
test('one inline computeMulti argument defined', () => {
setupElement(false, {inlineMultiDep1});
assertEffects({computeMulti: 1});
assert.equal(el.$.child.computedMulti, '[inlineMultiDep1,undefined]');
});
test('all inline computeMulti argument defined', () => {
setupElement(false, {inlineMultiDep1, inlineMultiDep2});
assertEffects({computeMulti: 1});
assert.equal(el.$.child.computedMulti, '[inlineMultiDep1,inlineMultiDep2]');
});
});

suite('warn', () => {
Expand Down Expand Up @@ -183,6 +279,36 @@
el.multiProp2 = undefined;
assertEffects({multiPropObserver: 1, warn: 2});
});
test('computeSingle argument defined', () => {
setupElement(true, {computedSingleDep});
assertEffects({computeSingle: 1});
assert.equal(el.computedSingle, '[computedSingleDep]');
});
test('one computeMulti argument defined', () => {
setupElement(true, {computedMultiDep1});
assertEffects({warn: 1});
assert.equal(el.computedMulti, undefined);
});
test('all computeMulti argument defined', () => {
setupElement(true, {computedMultiDep1, computedMultiDep2});
assertEffects({computeMulti: 1});
assert.equal(el.computedMulti, '[computedMultiDep1,computedMultiDep2]');
});
test('inline computeSingle argument defined', () => {
setupElement(true, {inlineSingleDep});
assertEffects({computeSingle: 1});
assert.equal(el.$.child.computedSingle, '[inlineSingleDep]');
});
test('one inline computeMulti argument defined', () => {
setupElement(true, {inlineMultiDep1});
assertEffects({warn: 1});
assert.equal(el.$.child.computedMulti, undefined);
});
test('all inline computeMulti argument defined', () => {
setupElement(true, {inlineMultiDep1, inlineMultiDep2});
assertEffects({computeMulti: 1});
assert.equal(el.$.child.computedMulti, '[inlineMultiDep1,inlineMultiDep2]');
});
});
});

Expand All @@ -209,6 +335,34 @@
el = fixture('declarative-multi-all');
assertEffects({multiPropObserver: 1});
});
test('computeSingle argument defined', () => {
el = fixture('declarative-single-computed');
assertEffects({computeSingle: 1});
assert.equal(el.computedSingle, '[a]');
});
test('one computeMulti arguments defined', () => {
el = fixture('declarative-multi-one-computed');
assertEffects({computeMulti: 0, warn: 1});
assert.equal(el.computedMulti, undefined);
});
test('all computeMulti defined', () => {
el = fixture('declarative-multi-all-computed');
assert.equal(el.computedMulti, '[b,c]');
});
test('inline computeSingle argument defined', () => {
el = fixture('declarative-single-computed-inline');
assertEffects({computeSingle: 1});
assert.equal(el.$.child.computedSingle, '[a]');
});
test('inline one computeMulti arguments defined', () => {
el = fixture('declarative-multi-one-computed-inline');
assertEffects({computeMulti: 0, warn: 1});
assert.equal(el.$.child.computedMulti, undefined);
});
test('inline all computeMulti defined', () => {
el = fixture('declarative-multi-all-computed-inline');
assert.equal(el.$.child.computedMulti, '[b,c]');
});
});
});

Expand Down

0 comments on commit ae1b417

Please sign in to comment.