Array splicing and dom-repeat lead to wrong path in deep property observers

[sawatsky]

If you have an array that is modified using this.splice, this.push, and this.set from the Polymer library and have a dom-repeat iterating over the array, and then do these steps:

1. Add some elements
2. Remove some elements
3. Change a property of any element physically AFTER the elements you removed

You will see that a deep observer of the array is given an incorrect path on the property change.

I believe this has to do with Polymer's use of delete this.store[key] instead of, say, this.store.splice(key, 1) in the removeKey function. The WeakMap isn't updated, and so future updates to the array are related back to mappings that no longer exist. Although I am not sure.

Example:

<dom-module id="test-element">
    <template>
        <template is="dom-repeat" items="{{tests}}">
            <!--Nothing needs to happen in here-->
        </template>
    </template>

    <script>
        Polymer({
            is: "test-element",

            properties: {
                "tests": {
                    type: Array,
                    notify: true,
                    value: []
                }
            },

            observers: [
                "_testsChanged(tests.*)"
            ],

            "_testsChanged": function(changed) {
                // With the dom-repeat, the wrong path is reported on subproperty change
                // Without dom-repeat, it's fine
                console.log("CHANGED:", changed, "PATH:", changed.path);
            },

            "add": function(value) {
                this.push("tests", { "value": value });
            },

            "remove": function(index) {
                this.splice("tests", index, 1);
            },

            "change": function(index, value) {
                this.set("tests."+index+".value", value);
            },

            ready: function() {
                this.runTest();
            },

            "runTest": function() {
                this.add("a"); // [{ "value": "a" }]
                this.add("b"); // [{ "value": "a" }, { "value": "b" }]
                this.add("c"); // [{ "value": "a" }, { "value": "b" }. { "value": "c" }]

                this.remove(0); // [{ "value": "b" }. { "value": "c" }]
                this.remove(0); // [{ "value": "c" }]

                this.change(0, "changed"); // [{ "value": "changed" }]

                // Expected path: tests.0.value
                // Actual path: tests.2.value
            }
        });
    </script>
</dom-module>

Notice the order of operations. Adding some elements, removing any but the last, then changing the property of the last.

[dfreedm]

The item's path in the collection is not the index of the in the array, but it is actually statically assigned when the item is added.

Here's a JSBIN where you can see that the items have the same key no matter their position in the array:
http://jsbin.com/bereba/edit?html,console

Also the relevant docs: https://www.polymer-project.org/1.0/docs/devguide/properties.html#array-observation

[ghost]

I have encountered a similar issue regarding the array observation path.
When adding an element in array, using splice method, and then change a subproperty from the added item, the path from array observation is not the correct one.
Let's say we have a persons array with 11 objects, each object having name and age as properties. When adding a person {name: Sim, age: 24} on position 2, and after change the property name of person Sim, the observer method returns the path "data.#11.name".
I expected the path to be "data.#2.name" because I added the item on the second position.

I made a jsfiddle for a better understanding. https://jsfiddle.net/simona_butnaru/zfwewthr/5/

I made a little research and I've seen that the issue is from polymer, at the add function from Polymer.Collection. This line (var key = this.store.push(item) - 1;) sets the key to be the length of array, and I think the key shoud be the position where the item was added in array.

[sawatsky]

@azakus I don't believe this issue should have been closed, as @simonabutnaru has shown another use case. What good is a static key to us? All other elements observing our array need the actual path to be able to react correctly.

Please inform us how to get the actual path in our array through the observer functions.

Surely we aren't expected to keep our own WeakMaps....

[arthurevans]

@simonabutnaru the JSFiddle you posted appears to be using a very old version of Polymer.

The data binding system requires static keys so it can, for example, match a data change to the elements generated by a dom-repeat. These keys are not supposed to match the element's position in the array. They're just supposed to be unique (it's an implementation detail that the initial set of keys match the initial positions.)

You can use the get and set APIs to access the elements referred to by a path. For example, if the path is persons.#12, you could do:

var person = this.get(path); // returns the item that corresponds to the path 'persons.#12'
this.set([path, 'age'], 12); // sets the value at 'persons.#12.age' to 12

[sawatsky]

@arthurevans This distinction is NOT obvious in the documentation.

This is obviously a question only for the people who designed the system but... What use do we, as users, have for the static key?

I can only assume that the design decision had to do with runtime efficiency. One the one hand, they could have given us, and dom-repeat, the path to the elements in OUR array (not the static key), and left dom-repeat to convert to the path containing the static key. On the other hand, we have the current system where the users, and dom-repeat, are given the static key path and it's the user's job to get the path for their array.

Which leads to the question: which is more efficient? Which operation will run more, dom-repeat or user operations?

I believe the answer would be dom-repeat (seems the obvious answer). And so I can understand the choice that the designers made. They will save themselves some runtime by removing the need for dom-repeat to convert paths.

However, as a user of the library, it is very frustrating to have this thing called "path" not be a path in our array. This distinction is NOT obvious in the documentation.

[arthurevans]

@sawatsky I totally agree our docs are inadequate in this area and we're working on it. Some small changes will land soon, but a complete revamp of the data binding docs is in the works. Currently, what information there is about data paths is scattered all over.

(FWIW, I believe your guesses on runtime efficiency would be spot-on. You can look up the array index, but it's an O(n) operation so we don't translate the key to an index proactively. We need to spell out how to accomplish common use cases, and provide a detailed reference on paths.)

[ghost]

@arthurevans I'm using the latest version of polymer in the application I'm working. I used that url in jsfiddle because I didn't find a url for the latest version of polymer.

In my case, I need the index from the path, and not the element itself, to notify the corresponding element from another array of objects. As @sawatsky said, it is very frustrating to receive a path that isn't the same from our array.

I used the get API to get the element from path, but I always get an undefined value. Maybe I should extract only "persons.#12" from "persons.#12.age", to get it work, as you have shown in your example, i don't know. Anyway, I hope you can find a way to resolve this.

[robdodson]

@simonabutnaru @sawatsky the docs are being updated with the following language to help explain this https://github.com/Polymer/docs/pull/1457/files#diff-07936cc3163b32c7b2a4f859ae877754R550

I updated the example a bit to show off usage of finding the key and array index
https://jsbin.com/wafipe/4/edit?html,output

I used polygit to load polymer into the example. @simonabutnaru you can use this service to always pull the latest copy of polymer into things like jsbin/jsfiddle.

Interestingly, I tried initially to observer persons.splices but then the observer seemed to run once with an undefined changeRecord (that feels like a bug?) and then during the actual array splice the changeRecord didn't have a base value so I couldn't use Polymer.Collection.get without referring to this.persons. It seems like the easiest thing to do is observe persons.* and filter on the changeRecord.path.

[kaste]

See #3239 for initial call being undefined when using *.splices. Not getting a base value is a new bug AFAIK.

[robdodson]

Thanks @kaste 👍

[ghost]

@robdodson It worked based on what is written in documentation. I still don't understand, why the number from the path isn't the position of the changed item from array?

Taking into account that 2 components are inter-communicating using data-binding, if the changed data path isn't the right one, the second component will fail to be updated.

Based on the example from jsfiddle with the array persons, if we will create a person-model component for every item from array as:

< template is="dom-repeat" items="{{persons}}">
< person-model data="{{item}}"></ person-model >
< /template >

the person-model component won't update, if the path for changed data isn't the right one. And in this case, I must do one more operation that I consider being unnecessary.

[robdodson]

@simonabutnaru I think some of your code got removed from your previous comment

[ghost]

@robdodson I edited the jsfiddle. https://jsfiddle.net/simona_butnaru/u2ok4m65/1/

I thought that, if the path is wrong, a different component of person-model will be updated. I expected the one corresponding to the index from path to be updated, not the actual item from array that was modified. I guess I was wrong...

[robdodson]

Yeah it's confusing. persons.#8 becomes a unique identifier for the item at array position 2. The unique key let's Polymer keep track of the item regardless of its position in the array. I almost wished that the unique key was more of a hash like cf23df2207d99a or something like that so it was really obvious that it had nothing to do with numeric position in an array.

I'm not clear from your previous comment if we've answered your questions or if you're still confused about something.

[ghost]

Yes, now I understand the path-key thing. Those 2 lines of code from documentation solved my problem.
Thank you!

[0rion7st]

Ruined full night on that misunderstanding of "#index" and "index" differences. Please change path naming conversion of array elements for something less confusing (i.e as @robdodson described in previous comment).
Also, for some reason my array element do not have .base property.
So in order to get key of array element used this fn:

function getIndexKeyHash(myArray, myIndex)
{
var collection = Polymer.Collection.get(myArray);
var keyIndexHash = collection.getKey(myArray[myIndex]);
return keyIndexHash;
}

[voondo]

I lost many hours because of this subtlety too...