Skip to content

Commit

Permalink
optimize iterations by caching keylist for publish and observe maps
Browse files Browse the repository at this point in the history
  • Loading branch information
Scott J. Miles committed Sep 12, 2013
1 parent ec74922 commit 9dcc388
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 26 deletions.
18 changes: 12 additions & 6 deletions src/declaration/prototype.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,20 @@
// get basal prototype
var base = this.generateBasePrototype(extendee);
// chain observe object
if (prototype.hasOwnProperty('observe') && base.hasOwnProperty('observe')) {
chainObject(prototype.observe, base.observe);
//console.log('observed:', prototype.observe);
if (prototype.hasOwnProperty('observe')) {

This comment has been minimized.

Copy link
@sorvell

sorvell Sep 12, 2013

Contributor

Since this is called before chaining, why do hasOwnProperty?

This comment has been minimized.

Copy link
@sjmiles

sjmiles Sep 12, 2013

Contributor

good catch

if (base.hasOwnProperty('observe')) {

This comment has been minimized.

Copy link
@sorvell

sorvell Sep 12, 2013

Contributor

Why do we only need to do this if base.observe is an own property?

This comment has been minimized.

Copy link
@sjmiles

sjmiles Sep 12, 2013

Contributor

mistake

chainObject(prototype.observe, base.observe);
}
// combine name list
prototype._observeNames = Object.keys(prototype.observe).concat(base._observeNames || []);
}
// chain publish object
if (prototype.hasOwnProperty('publish') && base.hasOwnProperty('publish')) {
chainObject(prototype.publish, base.publish);
//console.log('published:', prototype.publish);
if (prototype.hasOwnProperty('publish')) {

This comment has been minimized.

Copy link
@sorvell

sorvell Sep 12, 2013

Contributor

same questions as for 'observe'

if (base.hasOwnProperty('publish')) {
chainObject(prototype.publish, base.publish);
}
// combine name list
prototype._publishNames = Object.keys(prototype.publish).concat(base._publishNames || []);
}
// chain custom api
chainObject(prototype, base);
Expand Down
41 changes: 21 additions & 20 deletions src/instance/properties.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,29 +21,32 @@
var properties = {
// set up property observers
observeProperties: function() {
// TODO(sjmiles):
// TODO(sjmiles):
// we observe published properties so we can reflect them to attributes
// ~100% of our team's applications would work without this:
// perhaps we can make it optional somehow
//console.group('[%s]:observeProperties', this.localName);
// add observers as explicitly requested
for (var n in this.observe) {
//console.log('observable:', n);
var m = this.observe[n];
//if (this.publish && this.publish[n]) {
//this.observeBoth(n, m);
//} else {
this.observeProperty(n, m);
//}
//
// add user's observers
var n$ = this._observeNames;
if (n$) {
for (var i=0, l=n$.length, n; (i<l) && (n=n$[i]); i++) {
var m = this.observe[n];
if (this.publish && (this.publish[n] !== undefined)) {
this.observeBoth(n, m);
} else {
this.observeProperty(n, m);
}
}
}
// add observers for left-over published properties
for (var n in this.publish) {
//if (this.observe && !this.observe[n]) {
//console.log('attr-prop:', n);
this.observeAttributeProperty(n);
//}
// add observers for published properties
var n$ = this._publishNames;
if (n$) {
for (var i=0, l=n$.length, n; (i<l) && (n=n$[i]); i++) {
if (!this.observe || (this.observe[n] === undefined)) {
this.observeAttributeProperty(n, this.publish[n]);
}
}
}
//console.groupEnd();
},
_observe: function(name, cb) {
log.watch && console.log(LOG_OBSERVE, this.localName, name);
Expand All @@ -62,15 +65,13 @@
invoke.call(self, methodName, [old]);
});
},
/*
observeBoth: function(name, methodName) {
var self = this;
this._observe(name, function(value, old) {

This comment has been minimized.

Copy link
@sorvell

sorvell Sep 12, 2013

Contributor

should we consider memoizing these?

This comment has been minimized.

Copy link
@sjmiles

sjmiles Sep 12, 2013

Contributor

I messed around with lots of ways of doing it, nothing was satisfying

self.relectPropertyToAttribute(name);
invoke.call(self, methodName, [old]);
});
},
*/
bindProperty: function(property, model, path) {
// apply Polymer two-way reference binding
return bindProperties(this, property, model, path);
Expand Down

0 comments on commit 9dcc388

Please sign in to comment.