Error in each implementation

[laPlacek]

In store2.js each is implemented following way:

each: function(fn, fill) {// fill is used by keys(fillList) and getAll(fillList))
    for (var i=0, m=_.length(this._area); i<m; i++) {
        var key = this._out(_.key(this._area, i));
        if (key !== undefined) {
            if (fn.call(this, key, this.get(key), fill) === false) {
                break;
            }
        }
        if (m > _.length(this._area)) { m--; i--; }// in case of removeItem
    }
    return fill || this;
},

MDN Storage.key() states that "The order of keys is user-agent defined, so you should not rely on it."

Reorder of elements can happen in the middle of for loop. Especially:

• reorder of elements can happen on adding or removing elements from session storage (example below)
• reorder can be caused by another card if two cards share storage

As an example please consider following code:

const printAll = () => {
    console.log('All Elements:');

    for(let i=0; i < window.sessionStorage.length; i++) 
        console.log(`${i}: ${window.sessionStorage.key(i)}`);

    console.log('\n');
}

console.log('ADDING ITEMS');
window.sessionStorage.setItem('one', 'one');
printAll();
window.sessionStorage.setItem('two', 'two');
printAll();
window.sessionStorage.setItem('three', 'three');
printAll();
window.sessionStorage.setItem('four', 'four');
printAll();
window.sessionStorage.setItem('five', 'five');
printAll();
window.sessionStorage.setItem('six', 'six');
printAll();
window.sessionStorage.setItem('seven', 'seven');
printAll();
window.sessionStorage.setItem('eight', 'eight');
printAll();
window.sessionStorage.setItem('nine', 'nine');
printAll();
window.sessionStorage.setItem('ten', 'ten');
printAll();

console.log('REMOVING ITEMS');
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));
printAll();
window.sessionStorage.removeItem(window.sessionStorage.key(0));

For me it yields:

ADDING ITEMS
All Elements:
0: one

All Elements:
0: one
1: two

All Elements:
0: one
1: two
2: three

All Elements:
0: one
1: two
2: three
3: four

All Elements: // shift when adding!!
0: one
1: two
2: five
3: three
4: four

All Elements: // shift when adding!!
0: six
1: one
2: two
3: five
4: three
5: four

All Elements: // shift when adding!!
0: six
1: one
2: seven
3: two
4: five
5: three
6: four

All Elements: // shift when adding!!
0: seven
1: four
2: three
3: six
4: one
5: two
6: eight
7: five

All Elements:
0: seven
1: four
2: three
3: six
4: one
5: two
6: nine
7: eight
8: five

All Elements: // shift when adding!!
0: seven
1: four
2: ten
3: three
4: six
5: one
6: two
7: nine
8: eight
9: five

REMOVING ITEMS
All Elements:
0: four
1: ten
2: three
3: six
4: one
5: two
6: nine
7: eight
8: five

All Elements:
0: ten
1: three
2: six
3: one
4: two
5: nine
6: eight
7: five

All Elements:
0: three
1: six
2: one
3: two
4: nine
5: eight
6: five

All Elements:
0: six
1: one
2: two
3: nine
4: eight
5: five

All Elements:  // shift when removing!!
0: eight
1: one
2: two
3: nine
4: five

All Elements:
0: one
1: two
2: nine
3: five

All Elements:
0: two
1: nine
2: five

All Elements: // shift when removing!!
0: five
1: nine

All Elements:
0: nine

Conclusion: Shifting on add happens more often than shift on remove, but both can occur. for loop inside each implementation should not assume any order of elements in between loop operations.

[nbubna]

Ugh. I'll have to find some time to consider alternative implementations.

[nbubna]

Playing around with this a bit today. Interestingly, using sessionStorage seems to be problematic, but localStorage is stable. I can't even get items back in order from sessionStorage after a second write, much less a remove.

It's hard to care about trying to maintain order during each() when order is never maintained to begin with. It seems pretty clear that there is no order maintained in the storage implementation to begin with.

[nbubna]

Do you have a particular failing test case where the each implementation introduces a problem that isn't already in the underlying storage?

I mean, i can create tests where unpredictable things happen by deleting or adding to sessionStorage during an each loop, but it's not clear that that is a bug in the implementation, so much as a shortcoming of working with sessionStorage. I can rewrite each to grab all the keys first, then iterate, which would prevent the each fn itself from causing those issues, but in a multi-page scenario, you could still cause them, and even in the each fn, such an implementation could surprise by iterating over keys deleted in earlier iterations and still maybe missing keys added.

So, to sum, order maintenance isn't a thing to begin with, set/remove calls during each will always cause some kind of surprising behavior, and i think there's really nothing to fix here, just a choice between different sorts of problems. So for now, i'm closing this. If you (or someone else) wants to make a case for a different implementation (like gather keys, then iterate), i could be persuaded, but for now, i think the best thing to do is leave it be.