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

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

Closed
haxxxton opened this issue Nov 17, 2016 · 6 comments
Closed

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

haxxxton opened this issue Nov 17, 2016 · 6 comments

Comments

@haxxxton
Copy link

I'm submitting a ...

[ ] bug report => search github for a similar issue or PR before submitting
[x] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior
Currently, when a (very) deep object is thrown as part of an error. The error message attempts to stringify the object, this freezes browsers for a significant period of time, and then produces an error message, many thousands of lines long. See screenshot

Expected behavior
If there were a default max depth to stringify to, a produced error message would only stringify to this depth from the original object.

Minimal reproduction of the problem with instructions
An object (parent) references many child objects in a key called children, each child references its parent object in a key called parent. Should an error be thrown on a child object (say a Duplicates in a repeater are not allowed error) the entire structure is stringified in the error section Duplicate value: .

What is the motivation / use case for changing the behavior?
Speedier error messages, that allow for quicker debugging.

Please tell us about your environment:
OSX 10.12.1, Sublime Text 3, BreezeJS

  • Angular version: 1.5.6

  • Browser: all

  • Language: all (as far as im aware)

@petebacondarwin
Copy link
Contributor

This sounds reasonable, minErr is calling toDebugString, which is responsible for serializing the object, which is already accounting for cyclic dependencies. We could add a parameter for depth too...

@gkalpak
Copy link
Member

gkalpak commented Nov 21, 2016

Reminder: If we change the minErr implementation, we need to also update ng-closure-runner.

@m-amr
Copy link
Contributor

m-amr commented Nov 24, 2016

I think i can do this by one of the following options

1- reimplement JSON.stringify with maxDepth parameter for example convertFromJsonToString

    function convertFromJsonToString(obj, maxDepth){/** implementation*/}

2 -implement clone The JSON with maxDepth then use JSON stringify (use more auxiliary space )

    function cloneJson(obj, maxDepth){/** implementation*/}
    var json = {a:{b:0}};
    var tempJson = cloneJson(json, 1);
    var jsonString = JSON.stringify(tempJson);

I think the second solution is easy to implement but it it will use more auxiliary spaces .

Please give me any suggestion about which solution i should use ?

@gkalpak
Copy link
Member

gkalpak commented Nov 24, 2016

I think the second option is preferrable. Re-implementing JSON.stringify correctly and with maxDepth support won't be trivial and will add several extra loc.

@m-amr
Copy link
Contributor

m-amr commented Nov 24, 2016

I changed angular.copy to support maxDepth while copping object

  function copy(source, destination, maxDepth) {/** implementation*/}

so we can clone object with max depth with consideration of backward compatibility if maxDepth is not supplied to the copy method it will copy without a depth.

Should this change be done in another pull request like feat(copy): add depth to copy method ?

Thanks.

@gkalpak
Copy link
Member

gkalpak commented Nov 25, 2016

@m-amr , thx for working on this. Let's move the implementation discussion to #15433.

m-amr added a commit to m-amr/angular.js that referenced this issue Nov 25, 2016

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
closes angular#15402
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 25, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 25, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 25, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 26, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 26, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 26, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 26, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Nov 27, 2016
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 21, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 22, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 22, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 22, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 22, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 23, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 23, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 23, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 30, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Jan 31, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Feb 1, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Feb 1, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Feb 2, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Feb 3, 2017
m-amr added a commit to m-amr/angular.js that referenced this issue Feb 7, 2017
@gkalpak gkalpak closed this as completed in a0641ea Feb 8, 2017
gkalpak pushed a commit that referenced this issue Feb 8, 2017
ellimist pushed a commit to ellimist/angular.js that referenced this issue Mar 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants