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

Adds restamp mode to dom-repeat. #4363

Merged
merged 4 commits into from
Aug 1, 2017
Merged

Conversation

kevinpschaaf
Copy link
Member

Fixes #1713

When restamp is true, template instances are never reused with different items. Instances mapping to current items are moved to their new location, new instances are created for any new items, and instances for removed items are discarded. This mode is generally more expensive than restamp: false as it results in more instances to be created and discarded during updates and disconnection/reconnection churn. By default, object identity is used for mapping instances to items. Set restampKey to provide key based identity.

When restamp: true is used, restampKey can be used to provide a path into the item object that serves as a unique key for the item rather than using object identity, for purposes of mapping instances to items. Instances for items with the same key will be reused, while all others will be either restamped or discarded.

@dfreedm
Copy link
Member

dfreedm commented Apr 4, 2017

@kevinpschaaf please rebase

@evorapa
Copy link

evorapa commented Jul 26, 2017

Is this PR still in progress? I see that the tests pass on chrome but timeout on firefox. Is there any ETA for this being functional?

The customElements.get function was undefined until
the polyfill resolved the HTMLImports.
Instead of waiting for the imports to resolve and
modifying the prototype, it was easier to extract
a function that does the logic, as the prototype
was not properly propagated in non-Chrome browsers
@TimvdLippe
Copy link
Contributor

@azakus Rebased and fixed the tests

Copy link
Member

@dfreedm dfreedm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dfreedm dfreedm merged commit 06c6ea0 into master Aug 1, 2017
@dfreedm dfreedm deleted the 1713-kschaaf-repeat-restamp branch August 1, 2017 22:53
@dfreedm dfreedm restored the 1713-kschaaf-repeat-restamp branch August 9, 2017 22:49
@sushantbs
Copy link

Any updates about when this will be available? Have been waiting for this for a while.

@TimvdLippe
Copy link
Contributor

The change was reverted as @kevinpschaaf and @sorvell wanted to improve the API, but wasnt in the PR. We should be able to get the ball rolling again soon!

@sushantbs
Copy link

sushantbs commented Oct 26, 2017

@TimvdLippe, @sorvell Would be nice if you could let us know the reasons for reverting the PR and what are the use cases that the API needs to cater to.
Maybe I, or someone from the community, can possibly take this up?

Thanks for all the great work...

@TimvdLippe
Copy link
Contributor

@sushantbs Paging that to @kevinpschaaf and @sorvell who wanted to have some more discussion and tackle some issues. I am not aware of the enhancements they want to make to this PR 😄

dfreedm added a commit that referenced this pull request Nov 28, 2017
Fixes #1713

When restamp is true, template instances are never reused with different items. Instances mapping to current items are moved to their new location, new instances are created for any new items, and instances for removed items are discarded. This mode is generally more expensive than restamp: false as it results in more instances to be created and discarded during updates and disconnection/reconnection churn. By default, object identity is used for mapping instances to items. Set restampKey to provide key based identity.

When restamp: true is used, restampKey can be used to provide a path into the item object that serves as a unique key for the item rather than using object identity, for purposes of mapping instances to items. Instances for items with the same key will be reused, while all others will be either restamped or discarded.
@skortchmark9
Copy link

Would be nice to know what happened here.

@leebickmtu
Copy link

@TimvdLippe @kevinpschaaf @sorvell
So is restamp planned to be re-added at any point?

@kevinpschaaf
Copy link
Member Author

This implementation was put on hold, out of concern that the algorithm that was used to maintain key-instance identity was too simplistic and could have pathologically-bad performance as a result.

In the interim, a more complicated but performance-optimized algorithm was implemented in lit-html's repeat directive. Back-porting that to Polymer would be a significant amount of work and at this point in the Polymer library's lifecycle we realistically probably wouldn't implement such a significant new feature.

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.

dom-repeat is not properly updating elements on filter/sort
8 participants