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

dom-repeat doesn't call _render when an item changes #3626

Closed
43081j opened this issue Apr 28, 2016 · 23 comments · Fixed by #4942
Closed

dom-repeat doesn't call _render when an item changes #3626

43081j opened this issue Apr 28, 2016 · 23 comments · Fixed by #4942

Comments

@43081j
Copy link
Contributor

43081j commented Apr 28, 2016

Description

If you have a sorted dom-repeat and a change occurs to an item (such that the path is like items.#2), then _render will not be called internally (because of this).

This seems to be because it assumes any path which isn't items or items.splices must be a sub-property change, and thus should follow through to the logic for the observe property.

However, not all other paths are sub-property changes, clearly. Some can be of the form items.#2.

Should this not trigger a re-render? Or somehow check if the properties defined in observe have changed?

Live Demo

http://plnkr.co/edit/fACxm4GDp1RsLKerazjW?p=preview

Clicking the button will set one of the children to a new object, causing a items.#2 change notification.

As you can see, a workaround is in this demo, the observer manually calls render() if you uncomment it.

@arthurevans
Copy link

Yeah, it seems inconsistent that updating a single array item with this.set updates the display but not the sort. As a workaround, you could use this.splice to replace a single item—but it seems like Polymer should do the right thing here.

@ryanwtyler
Copy link

ryanwtyler commented Apr 28, 2016

Arthur, this is a problem when binding the array to a firebase-collection. Even if you use this.splice locally firebase-collection will call this.set('data', array[index])

@43081j
Copy link
Contributor Author

43081j commented Apr 28, 2016

Maybe it is enough to just check in the else block, if there is only one .. If there is, trigger _render.

However, it is possible you have set an item which is identical to the old one (just not by ref), so that would be a little inefficient in such cases.

@kaste
Copy link
Contributor

kaste commented Apr 29, 2016

As far as I always understood you need to set observe on dom-repeat. That might be clumsy or not, but otherwise you don't get a resort anyway. That said <template is="dom-repeat" ... observe="#"> should work AFAIR. If this is what you want, it is either not well documented, or I just use another bug as a works-for-me workaround, or whatever...

@ryanwtyler
Copy link

Kaste, the scope of observe is items.*.someProp. So it wouldn't work in this case even though an item in the array changed

@kaste
Copy link
Contributor

kaste commented Apr 29, 2016

Did you try? I'm talking about something like this http://jsbin.com/kesofavofu/edit?html,output

@ryanwtyler
Copy link

what is observe=# doing? or rather, why is it doing that?

@43081j
Copy link
Contributor Author

43081j commented Apr 29, 2016

@kaste that really is a nice hack you have there, but as always, a hack is a hack, not a solution.

@ryanwtyler the reason this works is the following code:

if(path.indexOf(paths[i]) === 0) {
// ...
    this.debounce('render', this._render, this.delay);
// ...
}

Of course "#2".indexOf("#") === 0 so kaste is relying on the fact dom-repeat isn't smart enough to check that the entire path chunk is equal but rather just the start of it contains the observed path.

Theoretically, this means if your items all have the properties fooBar and fooBaz, then observe="foo" will match and cause a re-render.

@kaste
Copy link
Contributor

kaste commented Apr 30, 2016

If nobody calls here, I'll do a fix.

@43081j
Copy link
Contributor Author

43081j commented May 1, 2016

The fix to the initial problem here, you think it would be sufficient to just check if the path is of form items.#2? If so, trigger a render, always.

Also, the condition I mentioned above can incorrectly match as per your hack. But I see why it works this way, because if the change path is a.b.c and your observe is a.b or a, it will match.

@kaste
Copy link
Contributor

kaste commented May 1, 2016

You're correct about the a.b.to a.b.c stuff. items.#2 must trigger render if there is a sort or filter fn. (Ok, that would be true for the other conditions as well.) Invariant: If subpath is the literal length abrupt.

@kaste
Copy link
Contributor

kaste commented May 1, 2016

It's actually, if you listen to foo.*, foo.bar.baz will trigger, if you listen to just foo only, foo.bar.baz will not trigger.

@43081j
Copy link
Contributor Author

43081j commented May 1, 2016

Ah true, if the path is items.length we probably should do nothing. As there will also have been an items.splices change at the same time, right?

As for foo.* vs. foo, are you sure? I just stepped through it and got the same behaviour.

It appears foo.* gets renamed to foo.. Then in the condition above, "foo.bar.baz".indexOf("foo") === 0 and "foo.bar.baz".indexOf("foo.") === 0, so both trigger a render.

@kaste
Copy link
Contributor

kaste commented May 1, 2016

Right, items.length can probably catched in _itemsChanged since we cannot forward this change to an instance anyway.

As for foo.*, I would go for the non-breaking change if possible. There is still the bug in the above conditional that foobar will trigger too.

Otherwise, throughout Polymer the following rules apply: Say you listen for foo.bar, foo and foo.barwill trigger, foo.baz and foo.bar.quod will not. If you listen for foo.bar.*, foo, foo.bar and foo.bar.quod will trigger, foo.baz will not. So since there is some special handling for .* in the observe observer I assumed the normal Polymer behaviour. If that's not backed up by some tests, I would probably go for deep observation by default.

43081j added a commit to 43081j/polymer that referenced this issue May 1, 2016
Fixes Polymer#3626 by triggering a render when a single item changes in a
sorted/filtered dom-repeat.
kaste added a commit to kaste/polymer that referenced this issue May 2, 2016
* For efficiency do not process `items.length` changes
* Ensure observing `foo` is not triggered by a `foobar` etc.
* Ensure assignments like `items.Polymer#2` etc. will trigger a resort/-filter

Fixes Polymer#3626.
@kaste
Copy link
Contributor

kaste commented May 2, 2016

@43081j Ahhrg, why didn't you say, you had some code. My approach: #3630.

There is something not right here and/or confusing, @arthurevans, cc @kevinpschaaf. Actually set('items.#2', obj) should not reuse the key #2, it should splice the array, removing #2 and adding (at the same index) a new object (with a new key). I mean that's the intent of the whole collections vs. arrays thing here. The keys ought to be stable. But... set allows this, and hence we have this issue in the dom-repeater not updating the view.

@43081j
Copy link
Contributor Author

43081j commented May 2, 2016

@kaste you can ignore it if you like, I had written it before discussing with you so it is likely not the best way of doing it :) I will comment on yours instead.

Also FYI, the observe property works like so:

  • foo will trigger on foo.bar, foo, fooBar
  • foo.* will trigger on foo.bar

& i guess you are saying that set('items.#2', obj) should actually splice items, such that this problem becomes solved as it will match the items.splices branch.

kaste added a commit to kaste/polymer that referenced this issue May 2, 2016
* For efficiency do not process `items.length` changes
* Ensure observing `foo` is not triggered by a `foobar` etc.
* Ensure assignments like `items.Polymer#2` etc. will trigger a resort/-filter

Fixes Polymer#3626.
@arthurevans
Copy link

I'm inclined to agree with @kaste here. this.set(array.#1, newItem) is essentially a splice operation, and I think it should be handled that way, but instead it re-uses the existing key:

// Special handling when last part is a array item: need to replace

Tagging @kevinpschaaf to review and comment. I don't know if this is short-term fixable or if we need to tell people to use this.splice as a workaround in the near term. (In particular, apparently firebase-collection does this.set('array.#key', newitem) so if this isn't going to be fixed in core it should be patched over there.)

@ryanwtyler
Copy link

https://github.com/GoogleWebComponents/firebase-element/blob/master/firebase-collection.html#L612

Firebase-collection is calling set('item.' + index, val). It's using the index. Not the key. Don't know if that matters

@kaste
Copy link
Contributor

kaste commented May 2, 2016

That matters, I confused both too. In the jsbin above I used items.2. If you literally set(items.#2,...) it would obviously very confusing if a later get(items.#2...) would not return the very same thing. 😕
I should have written set(items.2, ...) above, then obviously get(items.2, ...) should return your object, but the keys of the old object at index 2 and the new object are not the same.

@kevinpschaaf
Copy link
Member

I think there's a simple fix in _checkObservedPaths to ensure a path to an array item root (e.g. items.#2) always re-renders the list, regardless of whether _observePaths is set.

@kaste
Copy link
Contributor

kaste commented May 2, 2016

Sure, #3630

@mraerino
Copy link

Is this going to be fixed or should the use of splice be preferred?

@tjsavage tjsavage added the 1.x label Sep 8, 2016
@TimvdLippe
Copy link
Contributor

The change must probably be made in _handleObservePaths which should cause a render if the path at that point is an empty string aka it's a set of an array element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants