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

Bare minimum for custom effects (take 2) #3573

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
7 changes: 5 additions & 2 deletions src/lib/bind/accessors.html
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@

_effectEffects: function(property, value, effects, old, fromAbove) {
for (var i=0, l=effects.length, fx; (i<l) && (fx=effects[i]); i++) {
fx.fn.call(this, property, value, fx.effect, old, fromAbove);
if (!fx.disabled) {
fx.fn.call(this, property, value, fx.effect, old, fromAbove);
}
}
},

Expand Down Expand Up @@ -111,7 +113,8 @@
var propEffect = {
kind: kind,
effect: effect,
fn: Polymer.Bind['_' + kind + 'Effect']
fn: Polymer.Bind['_' + kind + 'Effect'],
trigger: property
};
fx.push(propEffect);
return propEffect;
Expand Down
2 changes: 2 additions & 0 deletions src/lib/template/templatizer.html
Original file line number Diff line number Diff line change
Expand Up @@ -286,12 +286,14 @@
_createHostPropEffector: function(prop) {
var prefix = this._parentPropPrefix;
return function(source, value) {
if (source !== prop) return;
this.dataHost._templatized.__setProperty(prefix + prop, value);
};
},

_createInstancePropEffector: function(prop) {
return function(source, value, old, fromAbove) {
if (source !== prop) return;
if (!fromAbove) {
this.dataHost._forwardInstanceProp(this, prop, value);
}
Expand Down
67 changes: 67 additions & 0 deletions src/standard/effectBuilder.html
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,73 @@
var prop = Polymer.Bind.addPropertyEffect(this, property, kind, effect);
// memoize path function for faster lookup.
prop.pathFn = this['_' + prop.kind + 'PathEffect'];
return prop;
},

_ensureOwnEffects: function(property) {
// Move the property effects from the prototype to the instance, so
// we can mutate them. We use prototypal inheritance here to go cheap.
var pE = this._propertyEffects;
if (pE) {
if (!this.hasOwnProperty('_propertyEffects')) {
pE = this._propertyEffects = Polymer.Base.chainObject({}, pE);
}

// Shallow-copy the effects array for this property, to the instance.
if (pE[property] && !pE.hasOwnProperty(property)) {
pE[property] = pE[property].slice();
}
}
},

addPropertyEffect: function(property, fn) {
this._ensureOwnEffects(property);

// We have to create the underlying trigger machinery here, if this is
// the first effect of the property.
var effects = Polymer.Bind.ensurePropertyEffects(this, property);
if (effects.length === 0) {
// The current value will be masked by the descriptor, read it ...
var val = this[property];
Polymer.Bind._createAccessors(this, property);
// ... and put it in our data store
this.__data__[property] = val;
}

// Theoretically the effects must be sorted again, but `function`-effects
// are executed last anyway, so we can skip this for now. NOTE that, if
// you add a custom effect inside another effect, t.i. the notification-
// loop is already running, mutating the effects in place like we do here
// would be usually catastrophic. This 'just' works b/c we prestore the
// loop-length in our for-loops (see `effectEffects`). Note as well that
// the newly added effect will not be executed before the next
// notification we have to propagate. This is intended and a bonus point
// for appending only.
return this._addPropertyEffect(property, 'function', fn);
},

removePropertyEffect: function(fx) {
// Since effects can be removed as a side-effect, t.i. within a running
// effects-loop, we must ensure not to shorten the effects-array in
// place, otherwise the for-loop would break. On the other side removing
// an effect must be immediate, so we use a flag here to indicate that
// this effect should be skipped.
fx.disabled = true;

// We then slice and splice and reassign the effects-array. This new
// array will be used not before the next notification we have to
// propagate.
var property = fx.trigger;
var effects = property && this._propertyEffects &&
this._propertyEffects[property];
if (effects) {
var index = effects.indexOf(fx);
if (index !== -1) {
effects = effects.slice();
effects.splice(index, 1);
this._propertyEffects[property] = effects;
}
}
},

// prototyping
Expand Down
14 changes: 10 additions & 4 deletions src/standard/notify-path.html
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,12 @@
var fx$ = this._propertyEffects && this._propertyEffects[model];
if (fx$) {
for (var i=0, fx; (i<fx$.length) && (fx=fx$[i]); i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: Must prestore length just like in accessors!

// use memoized path functions
var fxFn = fx.pathFn;
if (fxFn) {
fxFn.call(this, path, value, fx.effect);
if (!fx.disabled) {
// use memoized path functions
var fxFn = fx.pathFn;
if (fxFn) {
fxFn.call(this, path, value, fx.effect);
}
}
}
}
Expand Down Expand Up @@ -320,6 +322,10 @@
}
},

_functionPathEffect: function(path, value, effect) {
Polymer.Bind._functionEffect.call(this, path, value, effect);
},

_pathMatchesEffect: function(path, effect) {
var effectArg = effect.trigger.name;
return (effectArg == path) ||
Expand Down
4 changes: 4 additions & 0 deletions test/unit/bind-elements.html
Original file line number Diff line number Diff line change
Expand Up @@ -713,3 +713,7 @@
});
</script>
</dom-module>

<script>
Polymer({is: 'x-custom-effect'});
</script>
143 changes: 143 additions & 0 deletions test/unit/bind.html
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,149 @@
});
});

suite('custom user effects', function() {

test('Add custom effect', function() {
var el = document.createElement('x-custom-effect');

var called = 0;
el.addPropertyEffect('foo', function(path, value, old) {
called += 1;
assert.equal(path, 'foo');
assert.equal(value, 'bar');
assert.equal(old, undefined);
});

el.foo = 'bar';
assert.equal(called, 1);
});

test('Remove custom effect', function() {
var el = document.createElement('x-custom-effect');

var called = 0;
var fx = el.addPropertyEffect('foo', function() {
called += 1;
});

el.removePropertyEffect(fx);

el.foo = 'bar';
assert.equal(called, 0);
});

test('Ensure old values are sent', function() {
var el = document.createElement('x-custom-effect');
el.foo = 'bar';

var called = 0;
el.addPropertyEffect('foo', function(path, value, old) {
called += 1;
assert.equal(old, 'bar');
});

el.foo = 'quux';
assert.equal(called, 1);
});

test('Ensure path effects can be seen', function() {
var el = document.createElement('x-custom-effect');
el.foo = {bar: 'quux'};

var called = 0;
el.addPropertyEffect('foo', function(path, value, old) {
called += 1;
assert.equal(path, 'foo.bar');
assert.equal(value, 'quod');
assert.equal(old, undefined); // always undefined for structured paths!
});

el.set('foo.bar', 'quod');
assert.equal(called, 1);
});

test('Ensure independence', function() {
var el1 = document.createElement('x-custom-effect');
var called = 0;
el1.addPropertyEffect('foo', function() {
called += 1;
});

var el2 = document.createElement('x-custom-effect');

el2.foo = 'bar';
assert.equal(called, 0);
});

test('Ensure independence (property effects already created on prototype)',
function() {
Polymer({
is: 'x-custom-effect2',
properties: {
foo: {
observer: '_foo'
}
},
_foo: function(){}
});
var el1 = document.createElement('x-custom-effect2');

var called = 0;
el1.addPropertyEffect('foo', function() {
called += 1;
});
el1.foo = 'bar';
assert.equal(called, 1);

var el2 = document.createElement('x-custom-effect2');
el2.foo = 'bar';
assert.equal(called, 1);
});

test('Removing an effect as a side-effect does not break the running ' +
'effect loop', function() {
var el = document.createElement('x-custom-effect');

var called = 0;
var fx = el.addPropertyEffect('foo', function() {
el.removePropertyEffect(fx);
});
el.addPropertyEffect('foo', function() {
called += 1;
});

el.foo = 'bar';
assert.equal(called, 1);
});

test('Adding an effect as a side-effect does not invoke it immediately',
function() {
var el = document.createElement('x-custom-effect');

var called = 0;
var firstRun = true;
el.addPropertyEffect('foo', function() {
if (firstRun) {
el.addPropertyEffect('foo', function() {
called += 1;
});
firstRun = false;
}
});

el.foo = 'bar';
assert.equal(firstRun, false);
assert.equal(called, 0);

el.foo = 'quod';
assert.equal(called, 1);


});


});

</script>

</body>
Expand Down