loop over value keys to check wich value need to be updated #166
Conversation
mbleigh
left a comment
mbleigh
left a comment
There was a problem hiding this comment.
Thanks for taking a look at this! I have a few concerns:
- The performance of doing two
Object.keys().lengthon what could be arbitrarily large JSON blobs - This is going to solve things for a subset of cases -- all nested object properties will still trigger on every update.
@cdata I'd be curious to hear your thoughts on this.
firebase-document.html
Outdated
|
|
||
| // set the value if this.data does not exist or value is primitive or if firebase value obj contain less keys than this.data (https://github.com/Polymer/polymer/issues/2565) | ||
| if (this.__firstSync || !this.data || typeof value !== 'object' || ( Object.keys(value).length < Object.keys(this.data).length)) { | ||
| delete this.__firstSync; |
There was a problem hiding this comment.
Prefer this.__firstSync = false
There was a problem hiding this comment.
o.k. - will update that
|
|
||
| // now, we loop over keys | ||
| for (var prop in value) { | ||
| if(value[prop] !== this.data[prop]) { |
There was a problem hiding this comment.
This will always be true for sub-objects, no?
There was a problem hiding this comment.
yes it will - see my comment below for a short rationale
|
@mbleigh thanks for your comments, truly appreciated. For very large JSON blob, I would tend to think that other part of data processing will weight on performance way more than Object.key() (I am thinking of snapshot.val() in particular - which is process intensive and invoked by firebase-document on every firebase value update). Re. nested Object properties - yes they will still trigger on every update. I was reluctant to recurse down the entire tree because the problem this PR is trying to fix is to avoid observers to be fired every time a an update is made on a bound Object. A construct like |
| } | ||
|
|
||
| if (!this.new) { | ||
| if (!this.isNew) { |
There was a problem hiding this comment.
Should this be if (!this.isNew()) {?
There was a problem hiding this comment.
isNew is defined via its getter
get isNew() {
return this.disabled || !this.__pathReady(this.path);
},… for firebase-document: we need to set the data in this case as well
…t null. The old behavior caused data to be instantiated twice to zeroValue when path changed from null -> /userData//profile -> /userData/uid/profile
…t null. The old behavior caused data to be instantiated twice to zeroValue when path changed from null -> /userData//profile -> /userData/uid/profile
christophe-g
force-pushed
the
sync-only-updated-value
branch
from
cc94e13 to
c449d8b
Compare
|
|
||
| __pathChanged: function(path, oldPath) { | ||
| if (oldPath != null && !this.disabled && this.__pathReady(path)) { | ||
| if (!this.disabled && !this.valueIsEmpty(this.data)) { |
There was a problem hiding this comment.
we also want to handle the case where path is not a valid one, but oldPath was.
3 participants
Should resolve #88 and #56.