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

Eliminate collection. Fixes #3946 #3970

Merged
merged 21 commits into from
Sep 23, 2016
Merged

Eliminate collection. Fixes #3946 #3970

merged 21 commits into from
Sep 23, 2016

Conversation

kevinpschaaf
Copy link
Member

@kevinpschaaf kevinpschaaf commented Sep 20, 2016

Reference Issue

Fixes #3946: dom-repeat doesn't stamp correctly (which is a 2.x-specific issue; see below for related 1.x issues that this will also solve).

Description

This is a significant change that eliminates the Polymer.Collection abstraction from Polymer 2.x.

This abstraction aimed to assign unique keys to Arrays via a behind-the-scenes store associated per array via WeakMap, along with integration into Polymer's notifying array mutation API's. The abstraction allowed for two-way binding into elements repeated from arrays by virtue of stable keys providing unique paths to bound array items, with efficient updating of the rendered items via splices.

However, the system was inherently complex and in the end the theoretical efficiency gained did not pay for itself in terms of the cognitive overhead for things like key-based paths (items.#5.value), and a long tail of issues related to lack of support for duplicate array items and primitives via this approach. Likewise, as #3946 pointed out, it also was fundamentally incompatible with a major goal of the 2.0 release, which is to eliminate the requirement to use set/notifyPath (although the API remains and will still be more optimal in cases) by avoiding use of (default) object identity in Polymer's dirty-checking, since Collections were associated to arrays via object identity.

This change results in dom-repeat simply reacting to items or items.splices by iterating the items array and re-assigning item, index, and origIndex (index of item in original array prior to applying user sort/filter) for each change. This approach was already basically best-in-class on the dbmon benchmark which always took the "full refresh" code path, and still leaves room for more straightforward optimizations in the future.

Path notifications are still handled, but using the original index in the path where previously key was used (which is possible since the new rendering stores the itemsIdx<->instIndex mapping upon each change to items). The main breaking changes are that:
a. Polymer.Collection is now removed, and all array notifications are now sent directly with the index rather than the key
b. array.splices observers will still receive an object that only contains indexSplices, but will no longer contain keySplices

Note, this change also resolves (or makes moot) the following 1.x issues; it is likely that these will only be resolved in the 2.x branch due to the nature of the breaking changes required to resolve:

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot
Copy link

CLAs look good, thanks!

@sorvell
Copy link
Contributor

sorvell commented Sep 23, 2016

LGTM

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

Successfully merging this pull request may close these issues.

3 participants