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

angular.equals/merge does not support circular reference #14512

Closed
fernandoSaborio opened this issue Apr 25, 2016 · 7 comments
Closed

angular.equals/merge does not support circular reference #14512

fernandoSaborio opened this issue Apr 25, 2016 · 7 comments

Comments

@fernandoSaborio
Copy link

This is the same issue as reported in #2618, but for angular.merge and angular.equals.

An example can be seen at: http://plnkr.co/edit/j3qz1Rt3ABOXyWw0KpgB?p=preview

@gkalpak
Copy link
Member

gkalpak commented Apr 25, 2016

Similar issues/PRs have been "shot down" in the past (e.g. #7839, #11653). E.g. see #7839 (comment) for the reasoning.
So, I wouldn't get my hopes up...

There is also #10096 that would provide another way to solve this.

What is your usecase, btw ?

@fernandoSaborio
Copy link
Author

We are trying to optimize the uses of the $watchers in one application where the number are quite extensive, we were switching to use scope.$watch with objectEquality == true which relies on angular.copy and angular.equals, while doing so we found 3 scenarios in our application where objects with circular references were seem:

  • one is under the code we produce, but we haven't figure out how to remove it without loosing functionality.
  • the other two are caused by a third party library based on bootstrap internally handle by our client, we basically don't have an option of fixing these cases.

The optimization we have implemented works fine as long as the angular.equals doesn't find an object with a circular reference, but that is not the case, actually it is quite the opposite, circular references are find a lot, and we don't have too much options to actually fix the ones that are identified so far.

@gkalpak
Copy link
Member

gkalpak commented Apr 26, 2016

Switching to objectEquality: true hardy sounds like an optimization. It is the most expensive $watch form (after $watch(..., ..., false) and $watchCollection(...)).
Just saying...

@fernandoSaborio
Copy link
Author

hehehe, agree, but in our particular case it looks like the way to go, actually we have applied the change along with a patched version of angular.js to avoid circular references in DEV environment, and the approach of using objectEquality: true seems to be giving performance benefits that we need, still going to production with a patched version of angular.js doesn't seem to be feasible.

@arcreative
Copy link

arcreative commented Jun 8, 2016

It seems that one can patch this efficiently for their own use case if you know what you're looking for. In my application, I've written an ORM that uses circular structures liberally, but I know what comparisons fail, so I can short circuit angular.equals where necessary:

After if (t1 == t2 && t1 == 'object') {:

    // Patches for object equivalence in circular ORM structures
    if (o1.__type && o1.id && o1.__type == o2.__type && o1.id == o2.id) {
      return true;
    }
    // End patch

In my use case, there's a __type property and an id property, and in my implementation these always refer to the same object, though angular.equals disagrees. This is far more efficient and compatible for me than relying on WeakMaps or similar implementations.

Obviously patching the actual framework sucks, but it's better than being shot down and not having any solution whatsoever :-)

@robianmcd
Copy link

I'm running into the same issue. My use case is: when a user wants to edit a model I make a copy of if so that any changes can be canceled. Then when the user tries to save their changes I compare the copy with the original to see if anything actually needs to be saved.

@herrherrmann
Copy link

herrherrmann commented Sep 6, 2016

Same for me, I'm trying to do:

function updateEntity(entity, changes) {
    const entityToSave = angular.merge({}, entity, changes);
    return entityResource.update(entityToSave).$promise
        .then(updatedEntity => {
            entity = angular.merge({}, entity, updatedEntity);
            console.log('entity was updated.');
            return updatedEntity;
        })
        .catch(error => {
            console.error('entity could not be updated.');
            return error;
        });
}

… which would be very convenient but would include circular references.

Update: Using _.assignIn() for now.

Narretz added a commit to Narretz/angular.js that referenced this issue Jan 14, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 14, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 14, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 16, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 16, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 16, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants