googlearchive / observe-js

A library for observing Arrays, Objects and PathValues
1.35k stars 116 forks source link

fix bug with rootObj optimization in observed sets #73

Closed jmesserly closed 10 years ago

jmesserly commented 10 years ago

while investigating https://github.com/Polymer/polymer-dev/issues/101, I found two bugs with the rootObj optimization in observed sets:

jmesserly commented 10 years ago

@rafaelw and/or @arv mind taking a look?

jmesserly commented 10 years ago

Oh, by the way ... thoughts on sending this as a pull request instead of rietveld? Github has side-by-side view now so I've been pretty happy with it as a review tool, but curious what y'all think.

arv commented 10 years ago

LGTM but let @rafaelw have a look to since I haven't fully grok the implication yet.

jmesserly commented 10 years ago

friendly ping for this one & https://github.com/Polymer/observe-js/pull/74

(btw, thanks for the test comments @arv, will fix those before landing.)

jmesserly commented 10 years ago

going to land this as my other CL sort of depends on it (if I want to make the fix more general as suggested in https://github.com/Polymer/observe-js/pull/74#issuecomment-56866498). I'm pretty confident in this fix, but the main thing that worried me was a style issue: using a getter/setter for rootObject. I wasn't sure if using getters/setters is typical or if I should try and change how the newObservedSet closure is working (e.g. perhaps convert ObservedSet to the style of the other types in this file, where it puts the methods on the prototype instead of the instance?). Very happy to iterate on this though, so feel free to add post-submit comments.