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

fix(toJson): add ability to suppress exceptions #10065

Closed
wants to merge 2 commits into from

Conversation

kwypchlo
Copy link
Contributor

This is a pull request prepared for issue #9838

I have implemented solution discussed in #9838 and applied it in places that I was sure of.
There are several places left where toJson is used but I wasn't certain if exceptions should be suppressed there:

https://github.com/angular/angular.js/blob/master/src/ng/http.js#L135
https://github.com/angular/angular.js/blob/master/src/ng/http.js#L1098
https://github.com/angular/angular.js/blob/master/src/ng/interpolate.js#L257
https://github.com/angular/angular.js/blob/master/src/ng/filter/filters.js#L524
https://github.com/angular/angular.js/blob/master/src/ngCookies/cookies.js#L181
https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L871
https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L885
https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L1158
https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L1637
https://github.com/angular/angular.js/blob/master/src/ngScenario/Scenario.js#L80
https://github.com/angular/angular.js/blob/master/src/ngScenario/Scenario.js#L89
https://github.com/angular/angular.js/blob/master/src/ngScenario/Scenario.js#L298
https://github.com/angular/angular.js/blob/master/src/ngScenario/output/Json.js#L8

ps. yeah I know I did copy&paste on one line there and it looks awful but I didn't know if I could create new function in global angular scope like toJsonReplacer just to throw that line there.

Adds third parameter called options to toJson function (for the time being undocumented, private).
Adds support for suppressExceptions option (with test cases) that suppresses any exceptions that JSON.stringify might throw and returns undefined in such case.
This fix has been discussed on issue angular#9838
…e error messages for user

Makes use of suppressExceptions option parameter in toJson function.

Closes angular#9838
@petebacondarwin
Copy link
Contributor

Thanks for this PR @kwypchlo - I have made a few comments on the main commit. Can you look at them? I think the second commit is good. Regarding other places, I think we should avoid hiding exceptions as much as possible. We can always add in those at a later date.

@kwypchlo
Copy link
Contributor Author

@petebacondarwin thank you for your input, I have replied to you on the comments and I will update my PR as soon as I somehow learn how to commit new changes and squash them with the first, older commit :)

@caitp the thing is that people sometimes would like to log something like $log.error('You have no parameter of name 'hello' in object: ' + angular.toJson(object)) and they would expect to log at least something, certainly not throw an exception. Of cource they would be like 'why is my object undefined dammit' but at least they get first part of messege correct. I have suggested an idea above to create a function to decycle the object and try to print it again but it didn't get any love. Such function can be very simple, just loop over object properties recursively and check for circular reference or it can be more safe and complicated like https://github.com/douglascrockford/JSON-js/blob/master/cycle.js. It would produce something like that:

> var a = {}, b = {test2: a};
> a.test1 = b;
> JSON.stringify(JSON.decycle(a))
< "{"test1":{"test2":{"$ref":"$"}}}"

@caitp
Copy link
Contributor

caitp commented Nov 16, 2014

and they would expect to log at least something, certainly not throw an exception.

Yes, but the "something" that would be logged in this case would not be very useful to them, and certainly wouldn't be very useful in most cases where you want to JSON-ify an object (such as sending to a server). I don't think this should be a public API, and I don't think too much effort should be taken in implementing it.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Nov 17, 2014
Now that `minErr` can cope with objects that cannot be normally stringified
to JSON, just pass the error arguments straight through without trying to
stringify them first.

Closes angular#9838
Closes angular#10065
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Nov 17, 2014
Now that `minErr` can cope with objects that cannot be normally stringified
to JSON, just pass the error arguments straight through without trying to
stringify them first.

Closes angular#9838
Closes angular#10065
@petebacondarwin
Copy link
Contributor

I have provided an alternate fix in #10085 - @kwypchlo, @caitp can you take a look?

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Nov 17, 2014
Now that `minErr` can cope with objects that cannot be normally stringified
to JSON, just pass the error arguments straight through without trying to
stringify them first.

Closes angular#9838
Closes angular#10065
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants