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

Fix conditions when dom-repeat needs to re-sort or re-filter. #3630

Closed
wants to merge 1 commit into from

Conversation

kaste
Copy link
Contributor

@kaste kaste commented 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.#2 etc. will trigger a resort/-filter

Fixes #3626.

* 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 Author

kaste commented May 2, 2016

Note: this goes for least breaking change, and wildcard observing is the only option here, t.i. there is no difference between observing foo or foo.*.

There is something super-confusing to me. Actually set('items.#2', obj)set('items.2', obj) should not reuse the key, it should splice the array, removing what was at index 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.

@ryanwtyler
Copy link

@kaste if it didn't reuse the key I'd think you'd get some unexpected behavior with firebase-collection. If you make a change locally, this.set('item.1.prop', val), you'd expect item[1] to have same key before and after but it wouldn't because firebase would call this.set('item.1', obj) and splice in a new object with a new key.

@43081j
Copy link
Contributor

43081j commented Jun 7, 2016

FYI the new polymerfire elements fail badly with dom-repeat because of exactly this. They now iterate through the properties one by one and call set for each.

edit: polymerfire works fine i think

@devinivy
Copy link

The problem described here is very similar to an issue I logged a while back #3211

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

We would accept a change in 2.x that fixes this behavior, as dom-repeat significantly changed between 1.x and 2.x. 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.

@TimvdLippe TimvdLippe closed this Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants