Stricter JSON parsing of data-setup causes a SyntaxError ("Uncaught SyntaxError: Unexpected token ..."), but doesn't help track down the cause

[OwenEdwards]

Upgrading a plugin from 4.2 to 4.12, I spent a significant amount of time tracking down a SyntaxError message which turned out to be coming from JSON parsing of the data-setup parameter. It seems that the parsing is stricter, which makes sense, but the error reporting is not very helpful.

In #550, @heff mentioned:

The data-setup could be a JSON formatting issue. It needs to be formatted like data-setup='{"controls": true}' i.e. single quotes containing an object with every key in double quotes.

An error thrown by Video.js' own JSON parser was highlighted and addressed in #391 & #392. When Video.js uses the browser's built in JSON.parse (in my case, Chrome Version 41.0.2272.118 (64-bit) on OSX) to parse data-setup, the SyntaxError does not give any information about where this is happening (it seems like it might even be an error in the HTML!) Here's a reduced test case.

This line needs wrapping in a try ... catch:

https://github.com/videojs/video.js/blob/stable/src/js/player.js#L195

I can do a PR, but wanted to open this issue in case others run into the same problem.

[gkatsev]

Good catch. We definitely should be adding more info to the error that gets generated and not just fail.
Since we're switching to use browserify in vjs 5.0, we can switch to using safe-json-parse module to handle safe json parsing.

What should be the behavior in case of an error? Ignore the data-setup parameter and load the player as if {} was passed in?

[OwenEdwards]

I saw that you're improving JSON handling in 5.0, so figured it wasn't worth spending a lot of time fixing this!

Behavior in the error case is a good question. As with any error, detecting it and reporting it is the first priority; after that, I guess the two options are ignoring data-setup and loading the player (as you mention), or aborting loading the player. How does Video.js handle other errors, e.g. not finding a suitable tech (which would have been the outcome in my case if the error had been handled how you suggest)?

It's not much help for a visitor to a site to see an error like "invalid data-setup parameter", but if the page's author has the fallback <p class="vjs-no-js">To view this video please enable JavaScript, and consider upgrading to a web browser that <a href="http://videojs.com/html5-video-support/" target="_blank">supports HTML5 video</a></p> line, then that's not very helpful, either!

[gkatsev]

We've switched to use safe-json-parse in master. Now, it shouldn't fail here but we still need better error handling around JSON.parse.

[gkatsev]

Fixed by #2113