-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trailing Commas in JSON - Errors in IE8 #1943
Comments
That's interesting. It's working for me in IE8 without errors, at least on this demo page. http://vjs.zencdn.net/4.12.3/demo.html We would probably need to make this change in VTT.js for it to persist. Could you check if you see the same commas in that project? |
I've noticed this as well. This is only an issue in the non-minified code. |
Would either of you be willing to throw together a quick PR to fix it in VTT.js? |
If @EZWrighter does a PR, that would be cool and I'll get it merged into my build branch. |
My guess is your minified version goes through some kind of lint filter that automatically removes them. Yes, I checked the VTT.js project and it has illegal commas in it's JSON as well. @gkatsev, you will probably get to it faster than me. :-( |
I'm waiting for this pull request to get merged: mozilla/vtt.js#339 |
Looks like mozilla/vtt.js#339 got pulled in. @gkatsev is there anything left to do before we close this? |
Yes, I need to update our custom build. |
I should be able to get to it in the next couple of days. |
@gkatsev bump, any chance this can get landed? |
Working on it. Have the patched vttjs available here: https://github.com/gkatsev/vtt.js/tree/vjs-v0.12.1 |
I believe this is fixed now if you use one of the latest 4.12.releases. |
The latest release (4.12.3) has some json objects being created with trailing commas after the last item. Which is illegal and is causing IE8 to throw an error and not work.
The 3 extra commas are from vtt.js include section of the video.js library. I have manually removed the commas in my dev version and things work fine now.
Here is the regex to easily find them: ,[\s\r\n]*}
The text was updated successfully, but these errors were encountered: