Ensure viewMap doesn't leak from collection binding to first model instance

[nfm]

This fixes a bug where a view collection binding, whose child views had model bindings, would trigger events inappropriately on the collection when the model changed. As a result, the collection binding set() function would receive the model, and assume it was a remove event, even though the model had simply changed an attribute.

I've added a test case that reproduces the behaviour. Basically, when you change the model's attribute, the collection view for it is inappropriately removed from the DOM.

The viewMap variable was leaking from the collection binding to the first model in the collection, and addSourceToViewContext was inappropriately reusing events from the collection for the model.

This only occurred if the parent view was initialized with a non-empty collection. In the other test cases, where the collection is initially populated by resetting it, the bug was avoided.

[gmac]

Hmmm, this is pretty significant. Good catch. I'm going to hold off on accepting the pull request until I've had a chance to dig into this a little bit. Just blanking the reference seems like it could create its own set of issues with the parent not correctly getting its events after the child clears them. It may be that each view instance needs to manage its own mapping array to remain truly autonomous. I'll look through your test case this week and compare my findings with you. Either way, thanks for the report, and the fix candidate.

[gmac]

Hey @nfm, would you mind trying the fix I outlined above rather than setting viewMap=undefined on line 941? My proposed fix should maintain the existing flow of Epoxy internals, allowing each view to map to its own events array and then restore any parent mappings. I'd defer to your experience with the issue in question to know if this fix works as you'd expect it to.

[nfm]

@gmac I'll try your patch out today, it looks sensible to me.

[nfm]

@gmac The change from your comment above still suffers from the original issue (if I remove my viewMap = undefined modification). The test suite fails on the spec I added, too.

It's a particularly difficult bug to try to explain and I'm not sure if I've made it clear what's going wrong!

Here's a screenshot of the issue in progress, after applying your patch. I'm debugging it from Chrome, and the backtrace is from the spec I added.

The first two lines (where viewMap is undefined and source.cid is view12) are from the parent view's call to addSourceToViewContext. I then resumed execution. The second two lines are from the child view's call to addSourceToViewContext (this is view for the collection's first model having its bindings applied). As you can see, viewMap has leaked from the parent view - there are existing collection bindings in there. The first element in the array is the collection - sorry, I should have expanded it.

I think the correct state for the second two lines would be for viewMap to be undefined again, meaning that the child view doesn't have any bindings at the current point in execution. Am I on the right page there?

Does that make sense? Sorry, this is a real tricky one to explain and I can't tell if my description is just incomprehensible!

[gmac]

Thanks @nfm. I'll download your branch and run through it this week.

[sdemjanenko]

Has there been any decision on this issue? I ran into it as well.

[gmac]

I've pulled @nfm's changes into my local branch, made changes, and then done a comprehensive push to master. See 570dbb9 for my revised solution to @nfm's fix. The problem with what @nfm proposed was that it compromised the dependency map of the parent collection binding; ie: the collection binding sets up a dependency map array for itself and then starts building children, and then each child sets up a new dependency map for itself and overrides the original collection binding's map. I've adjusted the collection binding implementation to cache/restore its mapping state around the creation of child views. This should safely separate parent/child dependency graphs, while also keeping the parent graph intact.