feat(minErr): set max depth for angular error JSON stringify #15433

m-amr · 2016-11-24T21:29:29Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature

What is the current behavior? (You can also link to an open issue here)
#15402

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

closes #15402

gkalpak

On a high level it looks good. Still missing:

Tests.
Making the max depth configurable.

gkalpak · 2016-11-25T08:04:45Z

src/minErr.js

@@ -57,7 +58,7 @@ function minErr(module, ErrorConstructor) {

    for (i = SKIP_INDEXES, paramPrefix = '?'; i < templateArgs.length; i++, paramPrefix = '&') {
      message += paramPrefix + 'p' + (i - SKIP_INDEXES) + '=' +
-        encodeURIComponent(toDebugString(templateArgs[i]));
+        encodeURIComponent(toDebugString(templateArgs[i]), MAX_DEPTH);


MAX_DEPTH should be passed to toDebugString, not encodeURIComponent.

gkalpak · 2016-11-25T08:07:32Z

src/Angular.js

  var stackSource = [];
  var stackDest = [];
+  var currentDepth = 0;
+  maxDepth = (maxDepth && isNumber(maxDepth) && maxDepth > 0) ? maxDepth : -1;


I would simplify this: if (!isNumber(maxDepth)) maxDepth = NaN;

gkalpak · 2016-11-25T08:08:55Z

src/Angular.js

+
+  function copyRecurse(source, destination, currentDepth) {
+    currentDepth++;
+    if (maxDepth !== -1 && currentDepth > maxDepth) {


By setting maxDepth to NaN when not specified, this condition could be: if (currentDepth > maxDepth)

gkalpak · 2016-11-25T08:11:58Z

src/stringify.js

+    copy(obj, tempObj, maxDepth)
+  } else {
+    tempObj = obj;
+  }


This could be simplified to:

if (isNumber(maxDepth)) obj = copy(obj, {}, maxDepth);

gkalpak · 2016-11-25T08:19:38Z

src/Angular.js

+  function copyRecurse(source, destination, currentDepth) {
+    currentDepth++;
+    if (maxDepth !== -1 && currentDepth > maxDepth) {
+      return typeof source;


I would prefer some less ambiguous value that also makes it clear there was more stuff that has been truncated. Also, typeof can be misleading or at least not so useful, because almost anything that is not a primitive will return object.

If we wanted to keep it simple, maybe something like '...'. If we wanted to get more fancy, trying to get the name of the value's constructor and returning something like constructorName + '(...)' would be nice, but probably not worth it.

we are already using ... for cyclic references so I think that we should do the same for this.

gkalpak · 2016-11-25T08:24:39Z

src/minErr.js

@@ -34,6 +34,7 @@ function minErr(module, ErrorConstructor) {
  ErrorConstructor = ErrorConstructor || Error;
  return function() {
    var SKIP_INDEXES = 2;
+    var MAX_DEPTH = 1;


There should be a way to customize this (I am not sure what it should be) and the default should be a little higher imo (maybe 3 or 4).

I think to make max depth configurable we can,

var exampleMinErr = minErr('example'); throw exampleMinErr('one', 'This {0} is {1}', foo, bar);

Refactor exampleMinErr to be

throw exampleMinErr({code:'one', message: 'This {0} is {1}', messageArgs:[foo, boo]});

and we can keep both styles working.

Include the depth as a special string in the message like {$maxDepth=3}
'This {0} is {1} {$maxDepth=3}' so we get the depth

throw exampleMinErr('one', 'This {0} is {1} {$maxDepth=3}', foo, bar);

this Depth will be applied to all the message.

Or Include a depth for each placeholder like 'This {0:d8} is {1:d8} }'

throw exampleMinErr('one', 'This {0:d8} is {1:d8}', foo, bar);

d8 means that the depth for stringifying this object is 8.

I think the second solution is easier for implementation and have no backward compatibility Issue.

@gkalpak What is your opinion about that ?

I don't think it'll be useful in this case. See https://github.com/angular/angular.js/pull/15433/files#r97893306.

petebacondarwin · 2016-11-25T10:05:24Z

Wouldn't it be simpler and more performant just to provide a specialized replacer function to the JSON.stringify method, which tracked depth? Or is that not possible?

gkalpak · 2016-11-25T10:38:19Z

I couldn't think of a way to track depth from jsonReplacer (in a reasonable way) 😞

petebacondarwin · 2016-11-25T11:10:18Z

Yeah, fair enough.
This project https://github.com/Canop/JSON.prune ended up having to rewrite JSON.stringify completely!

Closes angular#15402 Closes angular#15433

m-amr · 2017-01-21T16:10:32Z

@gkalpak
I have added some unit tests and we can configure the max depth by passing it in the placeholder

var exampleMinErr = minErr('example');
throw exampleMinErr('one', 'This {0,maxDepth=2}', foo);

Could you please give this PR another look ?

Closes angular#15402 Closes angular#15433

m-amr · 2017-01-31T22:12:03Z

@gkalpak
I prefer your suggestion about using errorHandlingConfig as a getter and a setter,
I have changed the implementation to

function errorHandlingConfig(config) {
  if (isObject(config)) {
    if (isDefined(config.objectMaxDepth)) {
      objectMaxDepthInErrorMessage = isValidObjectMaxDepth(config.objectMaxDepth) ? config.objectMaxDepth : NaN;
    }
  } else {
    return {
      objectMaxDepth: objectMaxDepthInErrorMessage
    };
  }
}

so it will work as a setter when config (object type ) is supplied.
please tell me if there anything missing.

Closes angular#15402 Closes angular#15433

gkalpak · 2017-02-02T16:45:47Z

src/Angular.js

+ *
+ * @param {Object=} config - The configuration object. May only contain the options that need to be
+ *     updated. Supported keys:
+ * -     `objectMaxDepth`  **{Number}** - The max depth for stringifying objects. Setting to a


Why is the dash at the beginning of the line? Is it intentional?

I imagine it might have been cut-and-pasted from a diff?

Because when the dash is at the beginning of the line it will be converted to dot

which will be consistent with angular.bootsrtrap config docs

But when it is not at the beginning

Do you want to change it ?

What if you change line 148 to remove the indentation too?

Closes angular#15402 Closes angular#15433

m-amr · 2017-02-02T23:18:42Z

@petebacondarwin
@gkalpak
I have change it, it is now identical to the docs of 'angular.bootstrap' config in the identation
please tell me if it needs to change ?

Closes angular#15402 Closes angular#15433

gkalpak

We discussed this during the team meeting. I suggested two minor changes and then this willbe good to go.

gkalpak · 2017-02-07T16:21:55Z

src/Angular.js

@@ -807,6 +855,7 @@ function arrayRemove(array, value) {
 *                   Can be any type, including primitives, `null`, and `undefined`.
 * @param {(Object|Array)=} destination Destination into which the source is copied. If
 *     provided, must be of the same type as `source`.
+ * @param {Number=} maxDepth All properties of the source will be copied until reaching the max depth.


We discussed this internally and we decided that it is better not to make this parameter public at this point.
So, please remove any mentions from the copy docs.

gkalpak · 2017-02-07T16:42:32Z

src/Angular.js

@@ -125,6 +128,50 @@ var VALIDITY_STATE_PROPERTY = 'validity';

 var hasOwnProperty = Object.prototype.hasOwnProperty;

+var objectMaxDepthInErrorMessage = 5;


Please change this to be an object with one property per config option. E.g.:

var minErrConfig = { objectMaxDepth: 5 };

Closes angular#15402 Closes angular#15433

m-amr · 2017-02-07T17:47:44Z

@gkalpak
I did all the minor changes that you had requested.

Changed

var objectMaxDepthInErrorMessage = 5;

To be

var minErrConfig = {
  objectMaxDepth: 5
};

Removed maxDepth from copy docs

gkalpak · 2017-02-08T16:05:59Z

Regarding my earlier comment, it turns out that we don't need to change ng-closure-runner/minErr, because the changes in this PR are not applicable there.

…in errors configurable Closes #15402 Closes #15433

…in errors configurable Closes angular#15402 Closes angular#15433

googlebot added the cla: yes label Nov 24, 2016

gkalpak suggested changes Nov 25, 2016

View reviewed changes

gkalpak added this to the Backlog milestone Nov 25, 2016

gkalpak added component: misc core frequency: moderate needs: test needs: work severity: crash / freeze severity: inconvenient type: feature labels Nov 25, 2016

gkalpak mentioned this pull request Nov 25, 2016

Feature Request: set max depth for angular error JSON stringify #15402

Closed

m-amr force-pushed the stringify_json_with_depth branch 9 times, most recently from fa1823e to d53eca0 Compare November 27, 2016 16:03

m-amr force-pushed the stringify_json_with_depth branch from d53eca0 to 3ae6f6b Compare January 21, 2017 16:03

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 21, 2017

feat(minErr): set max depth for angular error JSON stringify

3ae6f6b

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from 3ae6f6b to f4ce0fc Compare January 22, 2017 08:20

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 22, 2017

feat(minErr): set max depth for angular error JSON stringify

f4ce0fc

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from f4ce0fc to 7afb730 Compare January 22, 2017 13:53

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 22, 2017

feat(minErr): set max depth for angular error JSON stringify

7afb730

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from 4d20c5c to fd75c4b Compare January 31, 2017 20:17

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 31, 2017

feat(minErr): set max depth for angular error JSON stringify

fd75c4b

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from fd75c4b to bae9191 Compare January 31, 2017 21:25

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 31, 2017

feat(minErr): set max depth for angular error JSON stringify

bae9191

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from bae9191 to 3da4484 Compare January 31, 2017 21:31

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 31, 2017

feat(minErr): set max depth for angular error JSON stringify

3da4484

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from 3da4484 to 467ca23 Compare January 31, 2017 21:37

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 31, 2017

feat(minErr): set max depth for angular error JSON stringify

467ca23

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from 467ca23 to 6c8a548 Compare February 1, 2017 00:33

m-amr added a commit to m-amr/angular.js that referenced this pull request Feb 1, 2017

feat(minErr): set max depth for angular error JSON stringify

6c8a548

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from 6c8a548 to d4b9de2 Compare February 1, 2017 00:52

m-amr added a commit to m-amr/angular.js that referenced this pull request Feb 1, 2017

feat(minErr): set max depth for angular error JSON stringify

d4b9de2

Closes angular#15402 Closes angular#15433

gkalpak mentioned this pull request Feb 1, 2017

Get rid of huge error messages #14744

Closed

gkalpak reviewed Feb 2, 2017

View reviewed changes

m-amr force-pushed the stringify_json_with_depth branch from d4b9de2 to d19b038 Compare February 2, 2017 23:10

m-amr added a commit to m-amr/angular.js that referenced this pull request Feb 2, 2017

feat(minErr): set max depth for angular error JSON stringify

d19b038

Closes angular#15402 Closes angular#15433

m-amr force-pushed the stringify_json_with_depth branch from d19b038 to 52de1d2 Compare February 3, 2017 00:12

m-amr added a commit to m-amr/angular.js that referenced this pull request Feb 3, 2017

feat(minErr): set max depth for angular error JSON stringify

52de1d2

Closes angular#15402 Closes angular#15433

gkalpak suggested changes Feb 7, 2017

View reviewed changes

m-amr force-pushed the stringify_json_with_depth branch from 52de1d2 to 15ff3e6 Compare February 7, 2017 17:15

feat(minErr): set max depth for angular error JSON stringify

15ff3e6

Closes angular#15402 Closes angular#15433

gkalpak approved these changes Feb 8, 2017

View reviewed changes

gkalpak closed this in a0641ea Feb 8, 2017

gkalpak pushed a commit that referenced this pull request Feb 8, 2017

feat(errorHandlingConfig): make the depth for object stringification …

4a5eaf7

…in errors configurable Closes #15402 Closes #15433

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017

feat(errorHandlingConfig): make the depth for object stringification …

8a28cae

…in errors configurable Closes angular#15402 Closes angular#15433

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(minErr): set max depth for angular error JSON stringify #15433

feat(minErr): set max depth for angular error JSON stringify #15433

m-amr commented Nov 24, 2016 •

edited

Loading

gkalpak left a comment •

edited

Loading

gkalpak Nov 25, 2016

gkalpak Nov 25, 2016

gkalpak Nov 25, 2016

gkalpak Nov 25, 2016

gkalpak Nov 25, 2016

petebacondarwin Nov 25, 2016

gkalpak Nov 25, 2016

m-amr Jan 21, 2017 •

edited

Loading

gkalpak Jan 25, 2017

petebacondarwin commented Nov 25, 2016

gkalpak commented Nov 25, 2016 •

edited

Loading

petebacondarwin commented Nov 25, 2016

m-amr commented Jan 21, 2017 •

edited

Loading

m-amr commented Jan 31, 2017 •

edited

Loading

gkalpak Feb 2, 2017

petebacondarwin Feb 2, 2017

m-amr Feb 2, 2017 •

edited

Loading

petebacondarwin Feb 2, 2017

m-amr commented Feb 2, 2017

gkalpak left a comment

gkalpak Feb 7, 2017

gkalpak Feb 7, 2017

m-amr commented Feb 7, 2017 •

edited

Loading

gkalpak commented Feb 8, 2017 •

edited

Loading

		@@ -125,6 +128,50 @@ var VALIDITY_STATE_PROPERTY = 'validity';

		var hasOwnProperty = Object.prototype.hasOwnProperty;

		var objectMaxDepthInErrorMessage = 5;

feat(minErr): set max depth for angular error JSON stringify #15433

feat(minErr): set max depth for angular error JSON stringify #15433

Conversation

m-amr commented Nov 24, 2016 • edited Loading

gkalpak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-amr Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Nov 25, 2016

gkalpak commented Nov 25, 2016 • edited Loading

petebacondarwin commented Nov 25, 2016

m-amr commented Jan 21, 2017 • edited Loading

m-amr commented Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-amr Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-amr commented Feb 2, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-amr commented Feb 7, 2017 • edited Loading

gkalpak commented Feb 8, 2017 • edited Loading

m-amr commented Nov 24, 2016 •

edited

Loading

gkalpak left a comment •

edited

Loading

m-amr Jan 21, 2017 •

edited

Loading

gkalpak commented Nov 25, 2016 •

edited

Loading

m-amr commented Jan 21, 2017 •

edited

Loading

m-amr commented Jan 31, 2017 •

edited

Loading

m-amr Feb 2, 2017 •

edited

Loading

m-amr commented Feb 7, 2017 •

edited

Loading

gkalpak commented Feb 8, 2017 •

edited

Loading