SC.Store.writeDataHash does not behave correctly with nested arrays. #865

Closed
krotscheck opened this Issue Nov 10, 2012 · 9 comments

Projects

None yet

3 participants

@krotscheck
Contributor

When a Nested ChildArray is constructed, it attaches three _kvo listeners to its datahash array: _kvo_array_did_change, _kvo_array_will_change, and _kvo_cloned.

If at some point in the future the ChildArray's parent record is refreshed via the writeDataHash method, the new dataHash is attached to the storeKey index for the parent record, however the _kvo listeners on the ChildArray are not moved over and remain attached to the old array.

I have two proposed solutions to this:
1- Go back in time and find the ChildRecord work that used to exist, and base a new solution on the assumption that a nested record structure should mirror the dataHash as closely as possible.
2- Update the writeDataHash method to go through all record attributes for each nested record and check for a toMany relationship, and update the array listeners.
3- Remove the need for the _kvo listeners altogether.

Also, some work needs to be done to ensure that the storekey pointers on nested records continue to point to the correct location, rather than the now-detached dataHash.

I also have about a weeks' worth of work that i can throw at this. If I do this though I'm going to go with solution 1, because from a theory standpoint it makes the most sense to me.

@dcporter
Member

I'm not at all up to speed on nested records, so this is shooting in the dark, but this sounds to me like an instance where something was created and then improperly destroyed. The parent record creates the ChildArray, right? If that's the case, then when the parent record creates the new one it ought to call .destroy() on the old one, and that method should include unhooking of its own _kvo observers. Thoughts?

@mauritslamers
Member

I would actually go for something resembling solution 2. The main problem (and the source of the _kvo* stuff) is the fact that the ChildrenAttribute (ChildArray) hooks observers into the stores datahashes, and the update process effectively destroys the observers data structure (but not the observers themselves!). So, effectively the best solution detects that observers exists, tries to find the observer itself and either removes or backups the observer, writes the update, and then reattaches / recreates the observer.

Of course, ideal would be to get rid of the observers onto the stores datahash. This could be done for example by having SC.Record keep a cached hash of the main record. This hash can then be used for observers, and SC.Record takes care of changes to the other records. Another way could be to have the store keep track of hashes and the SC.Record instances involved with it, so where the store now has a one-to-one relation between hashes and records, we keep track of multiple records per hash, instead of creating multiple hashes for one nested record.

@krotscheck
Contributor

Well, if the latter solution is the agreed-upon one, then I'm afraid I can't put the time towards it.

@dcporter
Member

@krotscheck I would encourage you to take a look at the create / destroy solution. If the observers are being hooked up on create, then it's the object's responsibility to unhook them on destroy. @mauritslamers may be right that there are underlying questions that could stand to be answered, but let's not let perfection get in the way of progress.

@krotscheck
Contributor

Well, unfortunately that issue was just deprioritized (much like anything that would ever allow us to contribute back to the framework), so my time to commit to this just evaporated. Sorry, guys.

@dcporter
Member

Sorry to hear it! The desire is greatly appreciated at least. =)

@mauritslamers
Member

I might try to fix it myself soon, depending on needs that might arise here..

Op 12 nov 2012, om 21:00 heeft Dave Porter het volgende geschreven:

Sorry to hear it! The desire is greatly appreciated at least. =)


Reply to this email directly or view it on GitHub.

@krotscheck
Contributor

Relevant discussion on this issue is on sproutcore-dev: https://groups.google.com/forum/?fromgroups=#!topic/sproutcore-dev/A259P2Aol5s

@dcporter
Member

Closing this out as a presumed duplicate to #1104.

@dcporter dcporter closed this Dec 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment