Refactors defaults for vtt.js #2426
Conversation
|
Both commits seem reasonable to me but I think the second would break the use case where someone is using the no-vtt version plus npm modules (to save file size upfront and lazy load vtt.js). Are you using the no-vtt.js version too or can you use the standard version that includes vtt.js already? |
| if (!window['WebVTT'] && this.el().parentNode != null) { | ||
| let script = document.createElement('script'); | ||
| script.src = this.options_['vtt.js'] || '../node_modules/vtt.js/dist/vtt.js'; | ||
| script.src = this.options_['vtt.js'] || 'https://raw.githubusercontent.com/mozilla/vtt.js/v0.12.1/dist/vtt.js'; |
There was a problem hiding this comment.
We're not using the official mozilla version right now.
Also, making this change means that developing locally could be a pain. Not sure exactly what we would want to do here instead. Maybe replace it with the rawgit url on built?
There was a problem hiding this comment.
Replacing on build would make sense. The built module should not have a reference to node_modules
There was a problem hiding this comment.
It may be a little... ugly... since it's not really what "versionify" implies, but we already have browserify-versionify in use. We could use that to replace the node_modules reference on build:
['browserify-versionify', {
placeholder: '../node_modules/vtt.js/dist/vtt.js',
version: 'https://raw.githubusercontent.com/gkatsev/vtt.js/vjs-v0.12.1/dist/vtt.min.js'
}],Confirmed this replaces it properly in the built version.
There was a problem hiding this comment.
Makes sense to do on built, though, that still doesn't solve the local development issue.
|
Hey, @brkattk, we're working on fixing the vtt.js inclusion thing in a new PR: #2741 |
|
@gkatsev I reset back to the first commit |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 56628df (Please note that this is a fully automated comment.) |
|
Hm... the test failure from @pam might be legitimate, though, possibly fixed already. Would you be able to rebase this branch against the latest master? |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 441e4cb (Please note that this is a fully automated comment.) |
|
Awesome. Looks like the tests are all green now (travis has stopped reporting back for some reason). |
|
LGTM |
No description provided.