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

moment.js and scope.$watch when objectEquality is true #4996

Closed
danielgatis opened this issue Nov 18, 2013 · 13 comments
Closed

moment.js and scope.$watch when objectEquality is true #4996

danielgatis opened this issue Nov 18, 2013 · 13 comments

Comments

@danielgatis
Copy link

Hi,
I have a object that have a moment.js property like this:

obj = { date: moment(), status: 'ok' }

so if i watch this object with objectEquality like this:

scope.$watch('obj', ..., true); 

i get this errors:
Error: [$rootScope:infdig] 10 $digest() iterations reached. Aborting!

To fix this i need to override angular.equals function to compare momentjs objects correctly.

function equals(o1, o2) {
  /* patch for moment */
  if (moment.isMoment(o1) && o1.toString() === o2.toString()) return true;
...
}

What is the best way to do this?

thanks.

@caitp
Copy link
Contributor

caitp commented Nov 18, 2013

It's not the angular equals function which is causing infinite digests --- these will happen when the actual value being compared changes too many times per digest cycle (sometimes called "model instability")

An example can be seen at http://plnkr.co/edit/AFnZYW403P4SN2xAvkWw?p=preview which intentionally modifies the value every time the $watch fires --- commenting out one of the two assignments in the $watch function will prevent from throwing.

@danielgatis
Copy link
Author

ok, but look at this example (http://plnkr.co/edit/rYxlCM?p=preview) no assignments happens in the $watch function and still throwing an error.

@heikki
Copy link

heikki commented Nov 18, 2013

Workaround without objectEquality: http://plnkr.co/edit/MjFelt?p=preview

@caitp
Copy link
Contributor

caitp commented Nov 18, 2013

ok, but look at this example (http://plnkr.co/edit/rYxlCM?p=preview) no assignments happens in the $watch function and still throwing an error.

No, interpolated expressions in text nodes are actually separate $watches --- In the case of this one in particular, it is actually executing an expression which evaluates to a new object every single time it is called. This, quite understandably, causes an infdig error to be thrown. The first $watch in your controller does not cause any problems, it is the {{moment.format(...)}} which does.

A good work-around for your issue is to write a filter for your momentjs needs, it's a very simple thing to do.

angular.module("moment", [])
.filter("time", function() {
  return function(input) {
    if (input instanceof Date) {
      return moment(input).format('hh:mm:ss')
    }
  }
});

like so

<p>{{ theDateInQuestion | time }}</p>

It's a pattern which works nicely

@danielgatis
Copy link
Author

@heikki
so clever

@caitp
Ok I got it. But how I will show it without interpolation?

@caitp
Copy link
Contributor

caitp commented Nov 18, 2013

As stated, you can use filters, this is really what they're meant to do --- transform input in some fashion (such as formatting a date). The filter chain won't cause an infdig error, and can be used very cleanly in any angular expression. They're good things to use

@danielgatis
Copy link
Author

ok! thanks

@heikki
Copy link

heikki commented Nov 18, 2013

The first $watch in your controller does not cause any problems, it is the {{moment.format(...)}} which does.

@caitp Are you sure about that? Removing the binding from the view still gives errors. I think the real reason is that angular objectEquality check doesn't work for moment object.

http://plnkr.co/edit/ejMlOT?p=preview

@caitp
Copy link
Contributor

caitp commented Nov 18, 2013

Moment objects are nothing special, they are just objects (Which happen to contain a Date, which is treated specially by angular.equals())

The cause of infdig errors is, a $watched expression returns a different result each time it fires (if it does this, it is triggered again, and again, and again, etc...) --- The maximum allowed number of tries is (by default) 10, so it doesn't take very long for it to throw.

You can see filters used to circumvent this in both of the examples I've posted.

Here's another one: http://plnkr.co/edit/MjNWFI?p=preview


Hmm, actually I retract that statement, it seems to end up with newval being a simple object, and oldval being the actual Moment instance --- So perhaps $watch does have issues with new'd objects.

Regardless, there are certainly ways to work around it (one being, simply store a Date object rather than a Moment)

EXCEPT, the first example uses the objectEquality flag watching a Moment object, and does not suffer from this issue: http://plnkr.co/edit/AFnZYW403P4SN2xAvkWw?p=preview

So, maybe I need to study your example closer. At any rate, there are ways around this which don't require silly hacks.

Use filters! It's what they're there for.

@dobladez
Copy link

Here's you last referenced example, with the only change that the moment instance is stored in $scope.model.moment instead of $scope.moment, and the deep watch is set on model: http://plnkr.co/edit/RSGc9b5WZJgRRccBQn2k?p=preview

This happens even if you don't even use the moment attribute in your template... it's purely an issue with angular's copy() and equals() behavior with MomentJS. The root of this, it seems, is that angular.copy() does not preserve the objects' prototype: #5063

Thanks!

@dcartwright
Copy link

I was just tripped by this bug as well, it's still present in 1.3.0 beta 5. I patched my copy of angular.js using the fix in #5063, as suggested by @dobladez, and that resolved the issue. In my opinion, this issue should be re-opened until #5063 is landed (if it is).

@danielgatis danielgatis reopened this Apr 9, 2014
@danielgatis
Copy link
Author

In my opinion, this issue should be re-opened until #5063 is landed (if it is).

I agree

@gentooboontoo
Copy link
Contributor

I am waiting for instructions if I should make #5063 backward-compatible with branch 1.2.x (and with IE8).

ckknight pushed a commit to ckknight/angular.js that referenced this issue Jul 16, 2014
So far, angular.copy was copying all properties including those from
prototype chain and was losing the whole prototype chain (except for Date,
Regexp, and Array).

Deep copy should exclude properties from the prototype chain because it
is useless to do so. When modified, properties from prototype chain are
overwritten on the object itself and will be deeply copied then.

Moreover, preserving prototype chain allows instanceof operator to be
consistent between the source object and the copy.
Before this change,

    var Foo = function() {};
    var foo = new Foo();
    var fooCopy = angular.copy(foo);
    foo instanceof Foo; // => true
    fooCopy instanceof Foo; // => false

Now,

    foo instanceof Foo; // => true
    fooCopy instanceof Foo; // => true

The new behaviour is useful when using $http transformResponse. When
receiving JSON data, we could transform it and instantiate real object
"types" from it. The transformed response is always copied by Angular.
The old behaviour was losing the whole prototype chain and broke all
"types" from third-party libraries depending on instanceof.

Closes angular#5063
Closes angular#3767
Closes angular#4996

BREAKING CHANGE:

This changes `angular.copy` so that it applies the prototype of the original
object to the copied object.  Previously, `angular.copy` would copy properties
of the original object's prototype chain directly onto the copied object.

This means that if you iterate over only the copied object's `hasOwnProperty`
properties, it will no longer contain the properties from the prototype.
This is actually much more reasonable behaviour and it is unlikely that
applications are actually relying on this.

If this behaviour is relied upon, in an app, then one should simply iterate
over all the properties on the object (and its inherited properties) and
not filter them with `hasOwnProperty`.

**Be aware that this change also uses a feature that is not compatible with
IE8.**  If you need this to work on IE8 then you would need to provide a polyfill
for `Object.create` and `Object.getPrototypeOf`.
grahame pushed a commit to muccg/angular.js that referenced this issue Jul 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants