Use "set" instead of "splice" for child changed

[louisptremblay]

When using firebase-collection in a dom-repeat, the whole dom-repeat has to refresh every time a child changes, instead of only the modified child. That's because it is updating the array with a splice.

If we use a "set" instead, only the actual child is refreshed.

The main problem with the "splice" was that I was losing focus on an input during the refresh.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[louisptremblay]

cla signed

[googlebot]

CLAs look good, thanks!

[johnlim]

Hi, I'm encountering the same problem which commit daad039 will fix. Will the team be incorporating this change? Thanks. :)

[ebidel]

Hmm, @kevinpschaaf isn't the data binding system supposed to preserve existing DOM if it's not changing?

[louisptremblay]

sorry, I added other commits by mistake to the pull request..

the only commit I wanted to add was daad039 "set" instead of "splice", the rest was for my own use, I'll clean this up and also fix the style

[louisptremblay]

I removed the non-related commits and fixed the style. Sorry again for the confusion

[kevinpschaaf]

@ebidel In general, dom-repeat attempts to remove-first-then-reuse instances when applying splices, so for a strict replace I would expect the result to be the same given set vs. splice, but I could be forgetting something.

@louisptremblay is there a quick jsbin repro you could create to show exactly how the behavior was causing you to lose input focus? I think your change is good, I'd just like to understand (/recall) exactly why that would happen.

[johnlim]

@kevinpschaaf In general, if I have an observer like so observers: ['_observeFirebaseCollection(firebaseCollection.splices)' ] that only wants to be notified when items are being added or removed, this observer would always be called when an item in the array is updated, which is not the expected behaviour.

[ebidel]

@johnlim can you throw together a jsbin using https://polygit2.appspot.com?

[johnlim]

@ebidel Sure, please see http://jsbin.com/xuyinu/edit?html,console,output If you modify the input values, you will notice from the console that the splice observer is called, which is not expected (or at least not what I'm expecting). In my context, I'm using the paper-datatable element, which does some dom manipulation on-dom-change (that is triggered by the splice), which causes the 'loss of focus of paper-input'. While this isn't strictly firebase-collection's 'problem', the splice does lead to situations with unintended side effects.

[louisptremblay]

just tweaking jsbin provided by @johnlim using a datasource with write access

• without proposed patch, input loses focus after typing one character: http://jsbin.com/wulutesupa/1/edit?html,console,output
• with the patch doesn't lose it: http://jsbin.com/denepitame/1/edit?html,console,output

[ebidel]

Huh, yea. That's not what I'd expect either. Maybe @kevinpschaaf could take a look.

We'll take the change in the PR. Thanks for the fix!

[kevinpschaaf]

@louisptremblay Got it, thanks. Your bin loses focus as a result of splice replacement because, even though the instance is reused by dom-repeat, it is detached and then reattached as part of the reuse code path, which causes the input to lose focus. :+1: on the change to use set, and it makes sense that this resolves the issue. Thanks!

[johnlim]

@ebidel @kevinpschaaf @louisptremblay Thank you for all your help. :)

[raydaly]

The master does prevent the error but it does not update any changes after the first change.

this.set('selectedAle.isKicked', true); this.set('selectedAle.kickedWhen', new Date().getTime());

isKicked gets updated in firebase but kickedWhen does not

I'm looking at line 343 var value = Polymer.Collection.get(change.base).getItem(key); and seeing the value does not contain the new Date().getTime()

I don't believe the issue should be closed or a new issue should be opened.

If after line 343 I added

value[change.path.split('.')[2]]=change.value;//update with change

then all changes got updated.

[ryanwtyler]

is there anyway to call set on the actual sub-property that changed instead of the entire object so that you can observe the subproperty that changed? I know firebase doesn't tell you what sub property changed but would it be practical to dirty check each sub-property for changes?