Bug in angular.equals following #7618

Following #7618 it is now possible to use objects which contains circular references (thanks for adding this). This test uses the Jasmine object equality check to verify that the copy of an object that contains circular references has indeed worked.

However, two such objects should also be angular.equals. Currently, this is not possible as the stack explodes:

it('should handle circular references when circularRefs is turned on', function () {
  var a = {b: {a: null}, self: null, selfs: [null, null, [null]]};
  a.b.a = a;
  a.self = a;
  a.selfs = [a, a.b, [a]];

  var aCopy = copy(a, null);
  expect(aCopy).toEqual(a);
  expect(equals(a, aCopy)).toBeTruthy(); // ** EXPLODES **
});

A fix similar to this does the job. Let me know if you want me to formulate a PR for this.

Thanks

Should have included a practical example of the impact of this bug in my first post.

With angular.equals broken in this way, it isn't possible to $watch objects that have circular references. A minimal example in test format would be:

it("should watch circular reference objects", inject(function($rootScope) {
  var a, spy;
  spy = jasmine.createSpy();
  $rootScope.$watch("name", spy, true);
  $rootScope.$digest();
  spy.reset();
  expect(spy).not.wasCalled();
  $rootScope.$digest();
  expect(spy).not.wasCalled();
  a = {};
  a.x = 5;
  a.a = a;
  $rootScope.name = a;
  $rootScope.$digest();
  return expect(spy).wasCalledWith(a, undefined, $rootScope);
}));

This fails with a Maximum call stack size exceeded error

Yes, do send a PR.

Here's my take on what you've already done:

You can follow a similar approach to angular.copy. No need for a equals_impl to be created. Simply make equals take 4 arguments and leave the last 2 parameters undocumented, just to stay consistent in the codebase.
Add specific tests to the equal section
I'm not sure why your comparisons array is two dimensional. Can't it just be two flat arrays named stackA and stackB, similar to .copy? I feel this is not needed and the implementation can be simpler.
Maybe I'm missing something. Some tests can prove me wrong :-)

Go ahead and send a PR. If you have any questions or can't make it, let me know.
It would be nice to have this fix before 1.3.0 gets released.

Great. Will pull together a PR tomorrow (taking into account your points) unless you need it before? Indeed what's the timeline for 1.3.0?

Created as #7839

In response to your points:

Thanks for the pointer confirming you're happy with that approach
Done
The reason I did this as a 2D array is largely for reasons of 'efficiency' (quotes intentional). Regardless of the size of object we are checking, we need to keep track of the pairs that we have encountered already. Given the symmetry of the equals relation, I keep track in both directions. So seen_list is a list of all the objects o at index i we have seen, comparisons at index i is the list of objects we have compared against o. Hence given two objects a and b, it is enough to check whether we have seen a and then if we have previously compared against b without then needing to check b == a. Difficult to quantify what 'efficiency' means here however, because there isn't per se a standard complex object against which we can benchmark.

Very happy to defer to a superior implementation however!

I responded here: #7839 (comment)

Navigation Menu

Repository navigation

Bug in angular.equals following #7618 #7724

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Bug in angular.equals following #7618 #7724

Description

Activity

myitcv commented on Jun 10, 2014

rodyhaddad commented on Jun 10, 2014

myitcv commented on Jun 12, 2014

myitcv commented on Jun 14, 2014

IgorMinar commented on Jun 27, 2014

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions