fix another off-by-one logic error for reordering

[neonstalwart]

fixes #191

cc @CRogers - thanks for the test case. please try this next chance you get. thanks.

[coveralls]

Coverage remained the same at 97.47% when pulling e34dd82 on neonstalwart:feature/191-keyed-correctness-tests into 3ea4f0e on Matt-Esch:master.

[CRogers]

Well, the good news is the particular test case seems fixed! But it still goes wrong for me with a 5x2 grid: http://jsfiddle.net/CARogers/o1m76ek8/4/. The good news is the number of disappearing nodes in my big application has noticeably reduced, so there's probably at least one other off by one error somewhere. I'll try and make some more test cases (and try to keep them as small as possible for our sanity).

[neonstalwart]

well... that's good and bad. I had a feeling there's still something lurking in there. thanks for your help.

[CRogers]

I've added a further test case to https://github.com/CRogers/virtual-dom/commits/feature/191-keyed-correctness-tests (there's some code duplication for the moment but eh). Interestingly, this one is kind of the inverse of the first one - we're removing every other element rather than adding them. If we're lucky this might actually cover all the cases.

[coveralls]

Coverage increased (+0.01%) to 97.48% when pulling d9bd81d on neonstalwart:feature/191-keyed-correctness-tests into 3ea4f0e on Matt-Esch:master.

[neonstalwart]

@CRogers can you try again? i fixed your test so that it would pass but in working my way through the code paths being exercised by your test i found another optimization we can do - we were doing some reordering that wasn't necessary if we accounted for the effect of nodes being removed. i never found any issue based on your test but i'll take an optimization as a win 😃. please let me know if you're able to come up with a test case that does cause a fail - feel free to continue to adjust the one you were already using since the assertions in there for the optimizations i made are now covered by other tests so you can completely do what you want with that test case.

@Matt-Esch i've adjusted the diff code so that reordering takes into account the index of what a node would be after removals have been applied (d9bd81d#diff-0a898b5225506bde82e3316215c895cdR250). by doing this, i was able to completely avoid doing a reorder during patch for cases that were just removals (d9bd81d#diff-4126e9547d4c816e13ec95414180672dR246). please take a look.

[CRogers]

Sorry about the bad test case (shouldn't code when tired). Sadly still looks borked, I added another couple of test cases at https://github.com/CRogers/virtual-dom/commits/feature/191-keyed-correctness-tests. Unfortunately they're a bit bigger but the smallest ones I could find that failed.

[Matt-Esch]

@neonstalwart I am going to pay close attention to this today. This is fundamental and can't be broken.

[neonstalwart]

I agree. my plan is to see if we can add some fuzzy testing that provides many more test cases than we could write by hand.

[CRogers]

Yeah, I basically found those two test cases by manually trying many combinations of the constants. This is something that could be automated really easily.

[neonstalwart]

FYI - I'm not currently working on this and don't expect I'll have time until next week. so if either of you have time this weekend then go ahead.

[Matt-Esch]

So I verified @CRogers tests on his feature/191-keyed-correctness-tests branch

Most of the problems with the tests are just bad tests. However after refactoring, I think there is a genuine issue with the last test.

It generates 10 items with keys [0, 1, 3, 4, 6, 7, 9, 10, 12, 13]. (Every third is missing). We then attempt to patch a full set of [0,..,14] onto it. The patch contains 5 inserts for [2, 5, 8, 11, 14] followed by a final order patch.

The contents of the order patch are

{
    "2": 10,
    "3": 2,
    "4": 3,
    "5": 11,
    "6": 4,
    "7": 5,
    "8": 12,
    "9": 6,
    "10": 7,
    "11": 13,
    "12": 8,
    "13": 9,
    "removes": {},
    "reverse": {
        "2": 3,
        "3": 4,
        "4": 6,
        "5": 7,
        "6": 9,
        "7": 10,
        "8": 12,
        "9": 13,
        "10": 2,
        "11": 5,
        "12": 8,
        "13": 11
    }
}

And the issue is that item 14 is out of place. So to run through the indices again

We start with

[0, 1, 3, 4, 6, 7, 9, 10, 12, 13]

We insert the 5 missing nodes at the end

[0, 1, 3, 4, 6, 7, 9, 10, 12, 13, 2, 5, 8, 11, 14]

We apply the final reorder and end up with

[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 14, 12, 13]

[neonstalwart]

ok, so if everything worked, we should see the insertBefore call happen with the following pairs of nodes

1. 2, 3
2. 5, 6
3. 8, 9
4. 11, 12
5. 14, null (or maybe not even at all)

this smells like some more bad accounting with insertOffset

[Matt-Esch]

A trace of the moves that actually happen (read this as insert <id> before <id>)

insert 2 before 3
insert 3 before 4
insert 5 before 7
insert 6 before 9
insert 7 before 9
insert 8 before 12
insert 9 before 13
insert 10 before 13
insert 11 before 14
insert 12 before null
insert 13 before null

The DOM ordering:

[0, 1, 3, 4, 6, 7, 9, 10, 12, 13, 2, 5, 8, 11, 14] <-start
[0, 1, 2, 3, 4, 6, 7, 9, 10, 12, 13, 5, 8, 11, 14] <- insert 2 before 3
[0, 1, 2, 3, 4, 6, 7, 9, 10, 12, 13, 5, 8, 11, 14] <- insert 3 before 4 (noop)
[0, 1, 2, 3, 4, 6, 5, 7, 9, 10, 12, 13, 8, 11, 14] <- insert 5 before 7
[0, 1, 2, 3, 4, 5, 7, 6, 9, 10, 12, 13, 8, 11, 14] <- insert 6 before 9
[0, 1, 2, 3, 4, 5, 6, 7, 9, 10, 12, 13, 8, 11, 14] <- insert 7 before 9
[0, 1, 2, 3, 4, 5, 6, 7, 9, 10, 8, 12, 13, 11, 14] <- insert 8 before 12
[0, 1, 2, 3, 4, 5, 6, 7, 10, 8, 12, 9, 13, 11, 14] <- insert 9 before 13
[0, 1, 2, 3, 4, 5, 6, 7, 8, 12, 9, 10, 13, 11, 14] <- insert 10 before 13
[0, 1, 2, 3, 4, 5, 6, 7, 8, 12, 9, 10, 13, 11, 14] <- insert 11 before 14 (noop)
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 13, 11, 14, 12] <- insert 12 before null
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 14, 12, 13] <- insert 13 before null

^ Uh wow...

[CRogers]

Mmm, those nulls would explain all the "can't call function insertBefore on undefined" errors I keep getting. Did either of you come up with any explanation about what might be happening?

[neonstalwart]

insertBefore null is not a problem - it means, add to the end of the list of childNodes - http://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#ID-952280727

reordering is being rewritten. the logic is moving to the diff phase so that the patch phase gets a list of moves like:

[
  { from: 6, to: 5 },
  { from: 4, to: 8 }
]

[neonstalwart]

with this rewrite there will be more tests... e.g. neonstalwart@f980c55

we (especially me) don't want to get this wrong again - please have patience while we use what little time we have to make this right.

[CRogers]

Oh excellent (I didn't notice the new branch) - thanks for the all the effort you're putting into this :)