Old items are not removed

[AdamWillden]

Best to show this as an example again: http://jsfiddle.net/pbgbj8pe/

See that old items are not removed when toggling skip and note what should be shown in the console log.

[AdamWillden]

Not sure if this is related to this issue or #5 but I've just looked at your processing of the array change event.

You must process the array change events by performing all deletes first followed by additions. See mbests comments here and my usage of the array change here

[brianmhunt]

Thanks @AdamWillden - those links were lifesavers. I think I have fixed this, and I will be uploading the change-set momentarily (and bumping to 0.2.2). The unit tests we now have in spec/ should make it easy to add test cases.

I will reference and close this issue via the comments, but if you are still experiencing issues I would be most grateful if you could report and re-open this.

Cheers

[AdamWillden]

@brianmhunt unfortunately the behaviour is not fixed (just tried 0.2.4) there's something unrelated to what I pointed you towards.

Check in the same jsfiddle (http://jsfiddle.net/pbgbj8pe), how do I reopen? Am I being blind?

[brianmhunt]

Many thanks @AdamWillden – Reopened. Let me have a look.

[brianmhunt]

@AdamWillden — The issue at hand is not jumping out at me. I am probably being dense, but could you perhaps give some more details as to what you are getting and what you expect?

Even better, if it is not too much trouble, would be to have the problem demonstrated in a unit test in spec/knockout-fast-foreach-spec.js – so we can definitively fix it. :)

Thanks & Cheers Adam

[AdamWillden]

I will try but I'm really pushing against a deadline at the moment (in fact my deadline came and went last Monday!). I'm interested in getting fastForEach working though.

You can see the expected current output in the console. If you keep clicking between the two buttons you'll see duplicate items start filling up too. The console output is key though.

[AdamWillden]

Here's a clearer output on the jsfiddle http://jsfiddle.net/rm2u2tub/

I'll try and do a unit test, looking a the existing unit tests I have a theory of what it might be. The example I have in the jsfiddle is the result of a computed where it's not the original array changing but instead producing a new array altogether. Perhaps it's something around that idea which is causing an issue.

I see there is a computed unit test but it's not checking it works for changes, just that it outputs initially.

[brianmhunt]

Thanks Adam. I'm out on the road right now, but will look at this ASAP.

On the difference between observable as computed, the trackArrayChanges extender should work exactly the same- hence only one test. That said I will definitely check out the lead and delve into it further as soon as I have a chance.

[AdamWillden]

Right, now I am suitably confused! I added a bunch of tests following the vein of the your observable tests and they all pass!

I'm at a loss. I'm definitely experiencing the issue in my main app in more than one place and it is evident in the fiddle. I'm fairly certain those tests replicate the fiddle use case too.

[AdamWillden]

Okay so doing a bit of debugging (I've not got chance to look the code over to get the solution).

Looking at this section of code this.lastNodesList only contains text nodes (nodeType 3) so ptr is a text node whereas this.element.children contain dom nodes (nodeType 1)...

Aha right I've sorted it, incoming PR ASAP

[AdamWillden]

Sent PR #9 - not sure how this issue wasn't picked up by the tests...

[brianmhunt]

Ok - looks like the delete issues are sorted. I made an oversight and was doing something quite silly and rather wrong, but it looks to be fixed now. Of course @AdamWillden if you see any silliness or wrongness hanging about, I would be most grateful if you could please advise! :)

Cheers

[AdamWillden]

Great work @brianmhunt. I'll try the latest dist and let you know if anything's not working!