Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

$watchCollection newValue always == oldValue #2621

Closed
mattlanders opened this issue May 9, 2013 · 58 comments
Closed

$watchCollection newValue always == oldValue #2621

mattlanders opened this issue May 9, 2013 · 58 comments
Assignees
Milestone

Comments

@mattlanders
Copy link

I trying to use the $watchCollection to watch an array, and I noticed that the newvalue is always equal to the oldvalue. After looking at the code, it looks like the values are copied to oldValue prior to firing the listener. It should be an easy fix.

@Soviut
Copy link

Soviut commented May 25, 2013

I noticed this too when trying to iterate over the oldCollection to remove Google Maps markers.

@appsforartists
Copy link
Contributor

Reduction:

http://jsfiddle.net/Y8yEZ/6/

@Soviut
Copy link

Soviut commented May 31, 2013

I think you mean "reproduction". I legitimately thought maybe you were talking about a "reduce" function at first ;)

@appsforartists
Copy link
Contributor

@Soviut http://www.webkit.org/quality/reduction.html

@Soviut
Copy link

Soviut commented May 31, 2013

Ah I see. I've always just called them test cases, examples or even proofs. I still stand by the fact that the language is ambiguous given that "reduce" is a common enough term general programming.

Thanks for the test case/example/proof though! ;)

@appsforartists
Copy link
Contributor

$watchCollection seems to be exceptionally flaky. Not only is there this unbelievably glaring issue (really, I'm not sure how a bug this big makes it to release at a place like Google), but in my limited testing, it seems to be behaving more like a deep watch than a shallow one. It's not my codebase, so I don't yet know if there's something particularly funky in the app, but I'm getting watchCollection calls just by changing the details of objects nested within the array I'm watching.

@Soviut
Copy link

Soviut commented May 31, 2013

There isn't much documentation on $watchCollection so the intent is a bit ambiguous. On first thought, I'd say that bubbling child events through the parent collection makes sense. That said, how do you only get it to watch the collection itself?

@appsforartists
Copy link
Contributor

$watchCollection: Shallow watches the properties of an object and fires
whenever any of the properties change (for arrays this implies watching the
array items, for object maps this implies watching the properties).

-- docs

If you want to deep watch, you can $scope.$watch(..., true).

For instance:

cart = []

$scope.$watch(cart, listenerA, false);     // standard watch
$scope.$watch(cart, listenerB, true);      // deep watch
$scope.$watchCollection(cart, listenerC);  // shallow watch

cart = [{'item': 'blackberries', 'quantity': 3}, {'item': 'pears',

'quantity': 1}];
// The value of cart is now different and all three listeners fire.

cart.push({'item': 'apples', 'quantity': 5});
// The value of cart is the same, but it contains a new value.

listenerB and listenerC fire.

cart[0].quantity++;
// The value of cart is the same and it has the same contents.  A

property on one of the items in the cart has changed. Only listenerB fires.

On Fri, May 31, 2013 at 1:12 AM, Soviut [email protected] wrote:
There isn't much documentation on $watchCollection so the intent is a bit
ambiguous. On first thought, I'd say that bubbling child events through the
parent collection makes sense. That said, how do you only get it to watch
the collection itself?


Reply to this email directly or view it on GitHub.

@appsforartists
Copy link
Contributor

Here's a new test for this:

Versal@2c83c08?w=0

appsforartists added a commit to Versal/angular.js that referenced this issue Jun 1, 2013
I took the naive approach suggested in angular#2621 of separating out the
changeDetected logic from the rest of the watch to give the listener
time to execute before oldCollection is overwritten.  The logic should
be identical to what's in the main repo, except no changes are made to
the closed-over state until after listener is called back.
Unfortunately, this doesn't appear to work, as the watchCollection
tests are now all failing.

I need to learn more about Karma so I can better debug this, and take
more time to grok the logic of the function before I'd have a shot at
fixing it.
@appsforartists
Copy link
Contributor

@mattlanders I followed your advice and moved all the side-effects from watchCollectionWatch into watchCollectionAction, leaving only the change detecting logic in watchCollectionWatch. Unfortunately, that doesn't seem to work. (The watchCollection tests are all failing now.)

I'd have to spend some more time reading and debugging this before I'd be able to submit a solid pull request.

@appsforartists
Copy link
Contributor

Alright. I'm getting close. Here's the work so far.

All the watchCollection tests are passing. Here are the failures:

FAILED ngRepeat stability should throw error on adding existing duplicates and recover 
FAILED ngRepeat stability should throw error on new duplicates and recover 

And here is the error causing those failures:

[NgErr27] 10 $digest() iterations reached. Aborting!  Watchers fired in the last 5 iterations: []

The fixed version of watchCollection seems to be triggering more digests than the old version. I had to add this line to keep duplicate lines from showing up in the watchCollection tests. I imagine that whatever was causing watchCollectionAction to be called multiple times in digest is the same thing causing these failures.

@appsforartists
Copy link
Contributor

Also, I added a note to the docs about newCollection and oldCollection being of different types. It would be a better choice if newCollection and oldCollection were always the same type (in this case, a POJO or an Array), but that has the potential to be a breaking change.

appsforartists added a commit to Versal/angular.js that referenced this issue Jun 5, 2013
I made a note in ticket angular#2621 about the type mismatch.
@ivoreis
Copy link

ivoreis commented Jun 24, 2013

Don't know if someone will need this, but it seems that this $watchCollection bug still present in future 1.1.6 release. As a way to temporary bypass this bug one could simply use the 'normal' $watch function like this:

     $scope.model.cart = [1,2,3];
     function getArray(){
       return $scope.model.cart;
     }
     function doSomething(newValues, oldValues){
      //...
     }
     $scope.$watch(getArray, doSomething, true);  //deep watch

@johnsoftek
Copy link

It's not doing a deep compare. It checks references for each high-level key (object) or index (array). The problem is that it makes a shallow copy from newValue to oldValue, so when the watch function returns, newValue = oldValue, kind of.
I made a quick fix,adding a third state called olderValue and returning newValue and olderValue from the listener. It works for me but I haven't run the official tests.

      $watchCollection: function(obj, listener) {
        var self = this;
        var olderValue;
        var oldValue;
        var newValue;
        var changeDetected = 0;
        var objGetter = $parse(obj);
        var internalArray = [];
        var internalObject = {};
        var oldLength = 0;

        function $watchCollectionWatch() {
          if (!isObject(oldValue)) {
              olderValue = oldValue;
          } else if (isArrayLike(oldValue)) {
              olderValue = [];
              for (var i = 0; i < oldLength; i++) {
                  olderValue[i] = oldValue[i];
              }
          } else {
              olderValue = {};
              for (key in oldValue) {
                  if (oldValue.hasOwnProperty(key)) {
                      olderValue[key] = oldValue[key];
                  }
              }
          }

          newValue = objGetter(self);
          var newLength, key;

          if (!isObject(newValue)) {
            if (oldValue !== newValue) {
              oldValue = newValue;
              changeDetected++;
            }
          } else if (isArrayLike(newValue)) {
            if (oldValue !== internalArray) {
              // we are transitioning from something which was not an array into array.
              oldValue = internalArray;
              oldLength = oldValue.length = 0;
              changeDetected++;
            }

            newLength = newValue.length;

            if (oldLength !== newLength) {
              // if lengths do not match we need to trigger change notification
              changeDetected++;
              oldValue.length = oldLength = newLength;
            }
            // copy the items to oldValue and look for changes.
            for (var i = 0; i < newLength; i++) {
              if (oldValue[i] !== newValue[i]) {
                changeDetected++;
                oldValue[i] = newValue[i];
              }
            }
          } else {
            if (oldValue !== internalObject) {
              // we are transitioning from something which was not an object into object.
              oldValue = internalObject = {};
              oldLength = 0;
              changeDetected++;
            }
            // copy the items to oldValue and look for changes.
            newLength = 0;
            for (key in newValue) {
              if (newValue.hasOwnProperty(key)) {
                newLength++;
                if (oldValue.hasOwnProperty(key)) {
                  if (oldValue[key] !== newValue[key]) {
                    changeDetected++;
                    oldValue[key] = newValue[key];
                  }
                } else {
                  oldLength++;
                  oldValue[key] = newValue[key];
                  changeDetected++;
                }
              }
            }
            if (oldLength > newLength) {
              // we used to have more keys, need to find them and destroy them.
              changeDetected++;
              for(key in oldValue) {
                if (oldValue.hasOwnProperty(key) && !newValue.hasOwnProperty(key)) {
                  oldLength--;
                  delete oldValue[key];
                }
              }
            }
          }
          return changeDetected;
        }

        function $watchCollectionAction() {
          listener(newValue, olderValue, self);
        }

        return this.$watch($watchCollectionWatch, $watchCollectionAction);
      },

@ryan65
Copy link

ryan65 commented Nov 24, 2013

Hi
Any news on this issue. are there any plans to fix it ?

@m0x72
Copy link

m0x72 commented Dec 16, 2013

Unfortunately this issue is still present in Angular 1.2.3. Would love to see a fix. Heads up :)

@xiehan
Copy link

xiehan commented Dec 20, 2013

+1
this is a pretty serious blocker for us

@allenhwkim
Copy link

1.2.5, still a bug. http://plnkr.co/edit/rfMCF4x6CmVVT957DPSS?p=preview

@alfonsodev
Copy link

👍

1 similar comment
@scchadwi
Copy link

+1

@caitp
Copy link
Contributor

caitp commented Dec 30, 2013

I'm curious... do you really want it to make a new copy of the collection just for the listener function? $watchCollection keeps a copy anyways, but only so it can spot differences. Seems like it would be a fair bit more expensive to be cloning the value every digest

@deakster
Copy link

I think at the very least, the API and docs should be updated then, because it is extremely misleading. People will implement behaviour based on the assumption that $watchCollection does what it says in the docs, and that it's listener receives a newCollection and an oldCollection. The documentation explicitly states:

The newCollection object is the newly modified data obtained from the obj expression and the oldCollection object is a copy of the former collection data.

This is clearly not the case, and hasn't been the case for a very long time, if ever.

@stivni
Copy link

stivni commented Jan 2, 2014

+1

Can I be of any help?

@siergiej
Copy link

siergiej commented Jan 3, 2014

+1

@ghost
Copy link

ghost commented Mar 7, 2014

+1

2 similar comments
@bendoh
Copy link

bendoh commented Mar 7, 2014

+1

@jared2501
Copy link

+1

@btford btford modified the milestones: 1.3.0-beta.2, 1.3.0-beta.1 Mar 10, 2014
@IgorMinar IgorMinar self-assigned this Mar 10, 2014
@ghost
Copy link

ghost commented Mar 14, 2014

+1

@tbosch tbosch modified the milestones: 1.3.0-beta.3, 1.3.0-beta.2 Mar 14, 2014
@thomfoolery
Copy link

+1

1 similar comment
@mwatt
Copy link

mwatt commented Mar 18, 2014

+1

IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Mar 18, 2014
Originally we destroyed the oldValue by incrementaly copying over portions of the newValue
into the oldValue during dirty-checking, this resulted in oldValue to be equal to newValue
by the time we called the watchCollection listener.

The fix creates a copy of the newValue each time a change is detected and then uses that
copy *the next time* a change is detected.

To make `$watchCollection` behave the same way as `$watch`, during the first iteration
the listener is called with newValue and oldValue being identical.

Since many of the corner-cases are already covered by existing tests, I refactored the
test logging to include oldValue and made the tests more readable.

Closes angular#2621
Closes angular#5661
Closes angular#5688
@dukehoops-zz
Copy link
Contributor

@IgorMinar - thank you for the fix. could you post in what release you think it will be rolled out?

IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Mar 18, 2014
Originally we destroyed the oldValue by incrementaly copying over portions of the newValue
into the oldValue during dirty-checking, this resulted in oldValue to be equal to newValue
by the time we called the watchCollection listener.

The fix creates a copy of the newValue each time a change is detected and then uses that
copy *the next time* a change is detected.

To make `$watchCollection` behave the same way as `$watch`, during the first iteration
the listener is called with newValue and oldValue being identical.

Since many of the corner-cases are already covered by existing tests, I refactored the
test logging to include oldValue and made the tests more readable.

Closes angular#2621
Closes angular#5661
Closes angular#5688
Closes angular#6736
IgorMinar added a commit that referenced this issue Mar 18, 2014
Originally we destroyed the oldValue by incrementaly copying over portions of the newValue
into the oldValue during dirty-checking, this resulted in oldValue to be equal to newValue
by the time we called the watchCollection listener.

The fix creates a copy of the newValue each time a change is detected and then uses that
copy *the next time* a change is detected.

To make `$watchCollection` behave the same way as `$watch`, during the first iteration
the listener is called with newValue and oldValue being identical.

Since many of the corner-cases are already covered by existing tests, I refactored the
test logging to include oldValue and made the tests more readable.

Closes #2621
Closes #5661
Closes #5688
Closes #6736
@IgorMinar
Copy link
Contributor

beta3 and 1.2.15, it's already out as a prerelease via bower or
https://github.com/angular/bower-angular/

On Tue Mar 18 2014 at 9:11:37 AM, Nikita Tovstoles [email protected]
wrote:

@IgorMinar https://github.com/IgorMinar - thank you for the fix. could
you post in what release you think it will be rolled out?

Reply to this email directly or view it on GitHubhttps://github.com//issues/2621#issuecomment-37952023
.

@sheerun
Copy link

sheerun commented Apr 25, 2014

Still at first firing of callback, old and new value are the same.. (instead old value being empty array)

Following helps:

return if oldValue == newValue

@johnsoftek
Copy link

Adam,

$watch has the same behaviour. It's intentional.

John M

On 25 April 2014 22:04, Adam Stankiewicz [email protected] wrote:

Still at first firing of callback, old and new value are the same..
(instead old value being empty array)


Reply to this email directly or view it on GitHubhttps://github.com//issues/2621#issuecomment-41385114
.

@caitp
Copy link
Contributor

caitp commented Apr 26, 2014

@johnsoftek that actually depends... It will have the same new and old value if the value changes in the same turn that the listener is registered, but if the listener is registered in one turn and then the value changes, the listener will fire with undefined for the old value.

I would question whether this is "intentional" or not, really. In watchtower we certainly don't intend for that behaviour

@XavierBoubert
Copy link

+1

@MaestroJurko
Copy link

newValue = oldValue ???

Why is then the watch triggered??

@lgalfaso
Copy link
Contributor

@mato75 look at the API for $watch

@jparmar
Copy link

jparmar commented Oct 9, 2014

+1

@johnsoftek
Copy link

@caitp $watch always calls the listener in the first digest after a watch is registered. It has a special check for first time that ensures it always return newvalue === oldvalue

@brianfeister
Copy link
Contributor

@IgorMinar, your comment is confusing me. Was this fixed in 1.2.15? Because I'm on 1.2.23 and I'm experiencing this issue.

@brianfeister
Copy link
Contributor

Very interesting thing I've observed just now. Don't fully understand the triggering conditions, but I was under the wrong impression that $scope.$watch if you point it to an array, was merely syntactic sugar for $scope.$watchCollection, turns out I was very wrong. initRun as discussed here was firing as expected but model dirty checking was failing so that newVall === oldVal each time the first execution happened. Changing from $scope.$watch ==> $scope.$watchCollection caused the model changes on first run to be recognized.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.