fix(minErr/ngRepeat/$rootScope): serialization of error objects

[petebacondarwin]

This is my preferred solution to #9838.
toJson should throw errors on cyclic object references. JSON is not really designed to handle this out of the box and we should not be hiding the error from the developer.
Adding a hidden parameter is just making this method convoluted and slowing it down.

Instead we should consider that for error messages, we do not high fidelity serialization of objects but instead a readable string that is useful for debugging. Returning undefined for broken JSON stringification is not helpful.

This fix provides a new function stringify which is available within the angular.js file and also within the angular-loader.js file. This function is then used inside minErr and $rootScope to provide nice string forms of objects.

Places that were using toJson to create error messages (i.e. ngRepeat and $rootScope) no either pass the object directly to minErr or they use the stringify method directly.

This solution ensures that we don't confuse the point of toJson - it is not a generic stringification tool, it is designed for converting to and from valid JSON. The new method stringify also provides us with a single future improvement point if we wished to make the stringification of objects to strings more comprehensive.

[caitp]

The circular reference detection scheme does not look right, you'd need to do a depth-first traversal of object properties and return them as strings to make that work. It's also not really a cheap operation, so we probably don't want to do that during interpolation.

[petebacondarwin]

I agree that it is not a real circular reference scheme. It is a basic algorithm that will get us a half decent error message.
Also, I agree that it should not be used in sensitive code. This fix only implements it for error messages.

[caitp]

You're right, $interpolate has its own stringify --- we should rename it to avoid confusion. How about toString()?

[petebacondarwin]

rename which one :-) ?

[petebacondarwin]

How about this new method is called toDebugString?

[petebacondarwin]

Actually the $rootScope usage is in hot code but it can be refactored nicely, which will actually even provide a minor performance enhancement as well.

[petebacondarwin]

I fixed up the $rootScope issue and renamed to toDebugString.

[kwypchlo]

I guess it looks fine, I like the idea to separate toJson and toDebugString.
In fact, now that the performance is not of such a big issue (it is just for debugging), I've come up with a decycle function that could be used there (it respects a straight-line reference):

function decycleObject(object, seen) {
  if (isObject(object)) {
    var safeObject = {};

    // make local copy of the reference array to be sure
    // objects are referenced in straight line
    seen = seen ? seen.slice() : [];

    for (var key in object) {
      var property = object[key];

      if (isObject(property)) {
        if (seen.indexOf(property) >= 0) {
          safeObject[key] = '<<already seen>>';
        } else {
          if(seen.indexOf(object) === -1) seen.push(object);
          safeObject[key] = decycleObject(property, seen)
        }
      } else {
        safeObject[key] = property;
      }
    }
    return safeObject;
  }

  return object;
}

You can see diffirence that it produce compared to your function in this plunkr. What do you think?

[kwypchlo]

Actually, I would also change <<already seen>> to << already seen >> because it distrupted the output of plunker when I tried to render it in html - it was treated as html tag.

[petebacondarwin]

Thanks @kwypchlo. I appreciate that it feels like I pulled the rug out from under you on this one. Sorry about that but it was as easy to implement it than to describe it.

How about we merge this and then you can provide PRs to make the changes you suggest?

We probably should go with something like [[ already seen ]] to ensure that it definitely doesn't get turned into HTML.

[petebacondarwin]

Or we could even just change it to "..." since this is only for error messages

[petebacondarwin]

The only other thing about the algorithm is that we don't want to add too many more bytes to the download.

[petebacondarwin]

Interestingly this PR actually knocks a little bit off the gzipped angular.min.js file (44.47Kb over 44.49Kb)

[kwypchlo]

@petebacondarwin don't worry about it :) I love angular and it feels good to help out anyhow 👍 Just go ahead with your PR and if you feel that the decycle function is worth the bytes, I can add it in in separate PR with tests. Also I guess "..." would be good enough for error messages.