Improve error handling in dojo.fromJson

[Matt Sgarlata]

There is no explicit error handling code in dojo.fromJson, so if you have some invalid JSON then you could be in for a nasty surprise. In FF I got "missing ) in parenthetical" and in IE I got "Line: 4, Error: Expected ')'". This can be very difficult to debug, and it would be immensely helpful if dojo could provide some help. Here is a revised version of the function with some explicit error handling. I am also including a test page where you can compare the current dojo behavior with the behavior of this improved function

dojo.fromJson = function(/*String*/ json){
	// summary:
	// 		Parses a [JSON](http://json.org) string to return a JavaScript object.  Throws for invalid JSON strings.
	// json: 
	//		a string literal of a JSON item, for instance:
	//			`'{ "foo": [ "bar", 1, { "baz": "thud" } ] }'`

	try {
		return eval("(" + json + ")"); // Object
	}
	catch (e) {
		console.warn("Invalid JSON: '" + json + "'");
		throw Error("Invalid JSON: " + e.message);
	}
}

[Matt Sgarlata]

Example page comparing existing and improved dojo.fromJson functions

[Matt Sgarlata]

BTW, I think this would have alleviated a lot of the pain and suffering from the user who submitted bug 7082: Dojo 1.0.2 to Dojo 1.1/1.1.1 migration - dojo.fromJson

http://bugs.dojotoolkit.org/ticket/7082

[Matt Sgarlata]

Screenshot of this error in an actual app. It's very difficult to see where the error is coming from

[dante]

keep in mind, try{}catch(){} is an expensive function, and likely not something (me personally at least) that should be used for convenience functions, but rather to actually trap occasional exceptions in edge cases.

[Matt Sgarlata]

I am no expert here, but according to the site below, try/catch is only expensive if the catch is executed.

http://dev.opera.com/articles/view/efficient-javascript/?page=2

So here the catch would only be expensive if there was in fact an error. So this would only be a concern if you were doing lots and lots of dojo.fromJson calls and lots and lots of them had errors.

I think you kind of hit the nail on the head there about convenience functions: they are to provide convenience to the user. In this case the convenience would be an error message letting the programmer know what's going on as opposed to just passing through a lower-level error.

[peller]

code size is another reason why we don't want try/catch/log, especially in base. Perhaps this method could be overridden / attached to provide this extra behavior in debug mode? Throwing an exception, though, is about as explicit as you can get when it comes to debugging. Unless the caller does something like squelch the exception, which is bad programming practice IMO, shouldn't this be easy to debug as-is?

[Les]

I agree this would be a helpful feature. I slightly modified the code as in Prototype.js:

dojo.fromJson = function(/*String*/ json){
	// summary:
	// 		Parses a [JSON](http://json.org) string to return a JavaScript object.  Throws for invalid JSON strings.
	// json: 
	//		a string literal of a JSON item, for instance:
	//			`'{ "foo": [ "bar", 1, { "baz": "thud" } ] }'`

	try {
		return eval("(" + json + ")"); // Object
	}
	catch (e) { }
	throw new SyntaxError("Invalid JSON string: " + json);

}

[Matt Sgarlata]

Replying to peller:

Perhaps this method could be overridden / attached to provide this extra behavior in debug mode?

That would work for me

Throwing an exception, though, is about as explicit as you can get when it comes to debugging. Unless the caller does something like squelch the exception, which is bad programming practice IMO, shouldn't this be easy to debug as-is?

I agree 100% throwing an exception is what should be done here. However, IMO exceptions should be appropriate to the layer in which they originate and higher layers should not simply propagate low level exceptions all the way up the stack. For example, if there is a low level network I/O problem that caused a database connection to fail, I would expect to receive a message along the lines of "database connection failed" not the low level I/O error. dojo.fromJson has a very specific semantic meaning, so if there is a problem with what is passed into that method then I believe dojo should make that clear by throwing a new exception that includes information about the original exception.

Particularly in this case, and in the case mentioned in bug 7082, the error messages provided from the browser are not specific enough to be meaningful. When the browser reports "missing ) in parenthetical" I know that means that I have a syntax error somewhere, but considering my entire application is JavaScript? this message really doesn't tell me a darn thing. Firefox might as well report "error somewhere in your application. have fun figuring out what it might be!"

Another advantage of this approach is that it will make the behavior of dojo.fromJson more uniform across browsers because right now the exceptions thrown by dojo.fromJson are browser dependent. With the new approach they will be a consistent type of error that can be counted on regardless of browser.

[peller]

Yes, the error message is inconsistent and cryptic, but that's not unique to fromJson -- try finding a syntax error in the loader, for example (and it doesn't stop there) I feel this is consistent with our coding style to avoid bounds checking, rethrowing exceptions, logging, etc. A debugger *could* catch the exception as it's thrown, though I agree a debug option to rethrow could be very helpful. I'd prefer we not add code like this to base (non-debug), however.