Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

fix bug with rootObj optimization in observed sets #73

Merged
merged 1 commit into from
Oct 9, 2014

Conversation

jmesserly
Copy link
Contributor

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

  • Models can have cycles, therefore, we may encounter the "root object" further down the path but not remember this fact. This leads to incorrectly failing to fire a change notification.
  • At first, my new test only failed when run by itself, but passed in the full suite. This is because of a second bug: lastObservedSet can sometimes be the set we just closed. This leads to confusion about whether the observed set was closed or not, ultimately leaving it pointing at the wrong rootObj in its closure. I tried to mitigate this in two ways: instead of this.object and rootObj both tracking the same value, this is now handled explicitly in a setter to ensure they stay in sync. Also, if we close an observer make sure it is not still listed as the (alive) lastObservedSet.

@jmesserly
Copy link
Contributor Author

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

@jmesserly
Copy link
Contributor Author

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.

var model = { a: {}, c: 1 };
model.a.b = model;

var called = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use counter instead of called to ensure the correct number of calls

var calls = 0;

@arv
Copy link
Contributor

arv commented Sep 25, 2014

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

@jmesserly
Copy link
Contributor Author

friendly ping for this one & #74

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

@jmesserly
Copy link
Contributor Author

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 #74 (comment)). 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.

jmesserly pushed a commit that referenced this pull request Oct 9, 2014
fix bug with rootObj optimization in observed sets
@jmesserly jmesserly merged commit 010658e into master Oct 9, 2014
@jmesserly jmesserly deleted the fix_root_obj_path branch October 9, 2014 20:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants