Invalid ng-repeat dupes error handling

[Toilal]

When value can't be converted to JSON due to circular structure, dupes checking in ng-repeat fails to display a proper error message.

See https://github.com/angular/angular.js/blob/d3b1f502e3099d91042a1827a006ad112ea67d36/src/ng/directive/ngRepeat.js#L368

toJson(value) call fails if value contains circual reference. This is causing this error message, and hides the proper error message : Duplicates in a repeater are not allowed. Use 'track by' expression to specify unique keys.

TypeError: Converting circular structure to JSON
    at Object.stringify (native)
    at toJson (http://localhost:9000/bower_components/angular/angular.js:1086:15)
    at ngRepeatAction (http://localhost:9000/bower_components/angular/angular.js:24042:42)
    at Object.$watchCollectionAction (http://localhost:9000/bower_components/angular/angular.js:13830:13)
    at Object.ng.config.$provide.decorator.$delegate.__proto__.$watch.applyFunction [as fn] (<anonymous>:778:50)
    at Scope.$get.Scope.$digest (http://localhost:9000/bower_components/angular/angular.js:13965:29)
    at Scope.ng.config.$provide.decorator.$delegate.__proto__.$digest (<anonymous>:844:31)
    at Scope.$get.Scope.$apply (http://localhost:9000/bower_components/angular/angular.js:14227:24)
    at Scope.ng.config.$provide.decorator.$delegate.__proto__.$apply (<anonymous>:855:30)
    at bootstrapApply (http://localhost:9000/bower_components/angular/angular.js:1487:15)

[kwypchlo]

Would it be a good idea to wrap JSON.stringify inside toJson function in try-catch block? In case of error in stringify, as in cyclic error, we could just return undefined.

It would of course imply a change that toJson doesnt throw anything anymore (which wasnt documented anyway) so if someone used something like this below, it would be a breaking change.

try{
angular.toJson(obj);
} catch(e) {
//dosomething
}

Other solution would be to use some extra function in places such as the one reported to check if the object has circular references and remove them before stringify ex. http://stackoverflow.com/questions/14962018/detecting-and-fixing-circular-references-in-javascript

I could prepare pull request if any of these solutions would be acceptable :+1:

[Toilal]

Thank you @kwypchlo .

[kwypchlo]

I prepared a fix so angular.toJson doesn't throw any exceptions and returns undefined if something went wrong but... I don't like it very much.

I thought about it a while and as a developer, if I would use angular.toJson(someObj) and it would return undefined and not a string represantation of my object, I would be like wtf?. The thing is, it actually should throw exception if someone passes object with circular reference because it is good for debugging. You should know that you have a circular reference and you should prepare your object for JSON.stringify so you get complete string representation of it. http://msdn.microsoft.com/en-us/library/ie/cc836474(v=vs.94).aspx

However, in some cases like yours, you never intended to use angular.toJson on your object, and you are completely allowed to have a circular reference inside it. Nevertheless angular wanted to give you feedback on some other error and passed your object to angular.toJson. In such case you shouldn't be slapped with and exception.

My new idea would be to add 3rd parameter to toJson called suppressExceptions, defaulted to false which would suppress exceptions. It would look like this:

function toJson(obj, pretty, suppressExceptions) {
  if (typeof obj === 'undefined') return undefined;

  try {
    return JSON.stringify(obj, toJsonReplacer, pretty ? '  ' : null);
  } catch(e) {
    // if suppressing exceptions, just return undefined
    if( suppressExceptions ) return undefined;
    throw e;
  }
}

Then we could find internal usages of toJson function inside angular and set suppressExceptions to true.

Actually even better idea would be not to return undefined in such cases but use some helper function that detects and removes circular references and leaves rest of the object alone. This would allow to show at least most of the object. I've seen similar approach in dojox and it could be adopted. https://github.com/dojo/dojox/blob/master/json/ref.js - example http://jsfiddle.net/dumeG/

What do you guys think?

[jeffbcross]

Like @kwypchlo already mentioned, the framework is built around the notion that toJson will throw on cyclic objects, which also helps developers find the source of problems quickly. So you're correct that suppressing by default is not a good approach.

I think this exact issue has been opened before, but I can't find it. It would be good to fix this, and I like the idea of adding a third argument to toJson. I think the implementation should only use try/catch if suppressExceptions is truthy though, since try/catch has a small performance penalty.

@petebacondarwin what do you think of adding a third parameter to toJson to suppress exceptions? This would technically be a new public API.

[jeffbcross]

Just chatted with @petebacondarwin, and here is the approach we want to take:

• Add a third options argument to toJson.
• Keep the argument private (undocumented) for now
• Implement logic to handle suppressExceptions property of options object, only using try/catch if the property is truthy.

A PR would be welcome @kwypchlo!

[kwypchlo]

@jeffbcross I'm going back from vacations tomorrow and I will gladly contribute and prepare PR :)