Make JSON.stringify call toJson() on objects that it can't serialize.

Make JSON.stringify call toJson() on objects that it can't serialize.

The toJson method must return a value that can be directly serialized
(a num/String/bool/Null/List/Map value).
Clean up of json.dart with more comments and less unnecessary privacy.

BUG=4716
Committed: https://code.google.com/p/dart/source/detail?r=12238

[Anders Johnsen, 2012-08-30 14:03:42]

LGTM, with comments.

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart
File lib/json/json.dart (right):

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:18: * Parsed JSON values are of the types [num], [String],
[bool], [Null],
Null -> null

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:33: * Returns 0 if [str] does not begin with a valid JSON
value.
Having this methods makes it look like we do some type of optimization do tell
how long the sting is, without doing the full work. Consider adding a note that
this is as time and memory consuming as stringify(...).length.

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:49: * If the object does not support this method, the
[NoSuchMethodError] thrown
Getting NoSuchMethodError from calling stringify(myObject) seams wrong to me. I
would prefer getting the JsonUnsupportedObjectType exception, saying toJson
didn't exist for the custom user-provided object.

[kasperl, 2012-08-30 14:14:14]

Can we still implement this interface with a native JavaScript JSON
implementation?

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart
File lib/json/json.dart (right):

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:35: static int length(String string) {
I really think this method should go away. It makes very little sense to me.

[floitsch, 2012-08-30 14:48:29]

LGTM with previous comments addressed.

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart
File lib/json/json.dart (right):

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:461: // Double values should create a representation with
sufficient digits to
This is guaranteed by toString.

[Lasse Reichstein Nielsen, 2012-08-31 14:29:18]

I think this new behavior can also be simulated through native JS
JSON.stringify, with suitable modifications to Dart objects, which we need to do
anyway for the current behavior.

It is already a problem to make the native stringify throw if a
non-primtive/List/Map object occurs in the value, as the current version does.
The JS stringify will happily convert *any* Dart object to an object literal. To
avoid that, the simplest solution is to put a toJSON JS method on all Dart
objects that throws if it's not directly serializable.
If we do that anyway, we can also make it check for a Dart toJson method and
call that to get a replacement. The JS stringify uses the same approach (calling
toJSON to get a replacement value) so it should be simple to hook into that, and
we needed to do it anyway, so it's not even extra costly.

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart
File lib/json/json.dart (right):

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:18: * Parsed JSON values are of the types [num], [String],
[bool], [Null],
"null" is not a type, "Null" is. I could rewrite it to talk about [num] values,
[String] values, [bool] values, the null value, etc., but I don't think it's
more readable.

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:33: * Returns 0 if [str] does not begin with a valid JSON
value.
Yes, it sucks. I don't know where it's used, or if it's needed, so I just
documented it instead of trying to change anything.
I don't want to specify that the function is inefficient. If it's a problem, we
can make a more efficient version that just scans through the string.
Or we could drop the function.

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:35: static int length(String string) {
Agree. It would be better to have a method that returned a JSON value iterator
over a string, so you could parse a sequence of values (which is what I think
this method is trying to support).
Or just remove it and see if there is a need at all.

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:49: * If the object does not support this method, the
[NoSuchMethodError] thrown
On 2012/08/30 14:03:42, ajohnsen wrote:
> Getting NoSuchMethodError from calling stringify(myObject) seams wrong to me.
I
> would prefer getting the JsonUnsupportedObjectType exception, saying toJson
> didn't exist for the custom user-provided object.

We can't test if there is a toJson method, we can only try calling it. We could
catch a noSuchMethodError and throw a JsonUnsupportedObjectType instead. It
wouldn't be absolutely correct - maybe there is a toJson method, but it's broken
and calls some other non-existing method. In that case, the noSuchMethod
exception would be correct, and if we catch it and convert it, that information
would go away. Or the toJson method would throw some other error that we don't
handle, so why make an exception for noSuchMethodError?

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:461: // Double values should create a representation with
sufficient digits to
I assumed that. I'll remove the comment, and check that double.toString has one.

[Lasse Reichstein Nielsen, 2012-08-31 14:55:44]

On 2012/08/30 14:14:14, kasperl wrote:
> Can we still implement this interface with a native JavaScript JSON
> implementation?
> 
> https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart
> File lib/json/json.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
> lib/json/json.dart:35: static int length(String string) {
> I really think this method should go away. It makes very little sense to me.

I think the wrapper class should go away too, and we should probably reconsider
the JSON API and make it more Dart-ish. Right now it's just a bad copy of JS
JSON.

[Lasse Reichstein Nielsen, 2012-09-11 13:10:10]

Please check the code in ddbg.dart

[Anders Johnsen, 2012-09-11 13:15:46]

LGTM, thank you Lasse!

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart
File lib/json/json.dart (right):

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:49: * If the object does not support this method, the
[NoSuchMethodError] thrown
On 2012/08/31 14:29:19, Lasse Reichstein Nielsen wrote:
> On 2012/08/30 14:03:42, ajohnsen wrote:
> > Getting NoSuchMethodError from calling stringify(myObject) seams wrong to
me.
> I
> > would prefer getting the JsonUnsupportedObjectType exception, saying toJson
> > didn't exist for the custom user-provided object.
> 
> We can't test if there is a toJson method, we can only try calling it. We
could
> catch a noSuchMethodError and throw a JsonUnsupportedObjectType instead. It
> wouldn't be absolutely correct - maybe there is a toJson method, but it's
broken
> and calls some other non-existing method. In that case, the noSuchMethod
> exception would be correct, and if we catch it and convert it, that
information
> would go away. Or the toJson method would throw some other error that we don't
> handle, so why make an exception for noSuchMethodError?

Discussed at office.

[Lasse Reichstein Nielsen, 2012-09-12 10:19:39]

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart
File lib/json/json.dart (right):

https://chromiumcodereview.appspot.com/10914009/diff/1/lib/json/json.dart#new...
lib/json/json.dart:49: * If the object does not support this method, the
[NoSuchMethodError] thrown
Doh, forgot to change the comment. I did change the code. :)

https://chromiumcodereview.appspot.com/10914009/diff/7001/lib/json/json.dart
File lib/json/json.dart (right):

https://chromiumcodereview.appspot.com/10914009/diff/7001/lib/json/json.dart#...
lib/json/json.dart:435: final String json;
I'll move the fields to the top of the class here too.

https://chromiumcodereview.appspot.com/10914009/diff/7001/lib/json/json.dart#...
lib/json/json.dart:463: // value of the string representation.
I forgot to remove this comment. Will do before commit.

[hausner, 2012-12-07 00:39:55]

I just now see that with this change, JSON.length() went away. I added that. I
can see that it was not efficient, but duplicating the JSON scanning code in
every script that needs this functionality is not a good solution either. I now
have to duplicate it again for the debugger test scripts. Why could it not stay
in the Json library?