feat: add external vhs properties and deprecate hls and dash references #859
Conversation
Deprecate lingering references to hls from the old videojs-contrib-hls project, as well as dash. Ensure all references use "vhs" instead. Remove unused utils/fixtures dir.
| @@ -16,11 +16,11 @@ QUnit.test('the environment is sane', function(assert) { | |||
| } else { | |||
| assert.strictEqual(typeof window.URL, 'function', 'URL is a function'); | |||
| } | |||
| assert.strictEqual(typeof videojs.Hls, 'object', 'Hls is an object'); | |||
There was a problem hiding this comment.
We probably want to keep this old section and add the updated lines as a new section.
There was a problem hiding this comment.
This may also be a good place to add checks for things that we want to update the name of but still be available via the old way. I'll spend some time looking through issues to see usage and add a comment if I find something interesting that we should test for.
There was a problem hiding this comment.
I see that there's some deprecation warning tests in the VHS.test.js file, so, this is probably fine as is in that case.
src/videojs-http-streaming.js
Outdated
| @@ -350,22 +354,37 @@ class HlsHandler extends Component { | |||
| if (!_player.hasOwnProperty('hls')) { | |||
| Object.defineProperty(_player, 'hls', { | |||
| get: () => { | |||
| videojs.log.warn('player.hls is deprecated. Use player.tech().hls instead.'); | |||
| videojs.log.warn('player.hls is deprecated. Use player.vhs instead.'); | |||
| tech.trigger({ type: 'usage', name: 'vhs-player-access' }); | |||
There was a problem hiding this comment.
should this trigger vhs-player-access? It sounds like this was used to track player.hls usage specifically. So, we probably don't want to trigger this here and only trigger hls-player-access.
Also, if we are un-deprecating usage of player.vhs, maybe we don't need vhs-player-access?
There was a problem hiding this comment.
Apparently player.vhs didn't have a deprecation warning on it while player.hls did.
Thinking about it some more, I think we should keep player.vhs and player.hls deprecated in favor of player.tech().vhs. Most users don't care about anything VHS related and it's highly dependent on the tech being used. I think accessing it via the tech makes sense and is safer.
Thoughts? Also, happy to pitch in and make the player.vhs -> player.tech().vhs changes in players.
There was a problem hiding this comment.
I think player.tech().vhs is fine (and makes sense), but I'd just be a bit worried that we do have a decent number of APIs and recommended calls that are at the vhs level (rather than at the video.js level) and if we recommend using player.tech().vhs people are going to see lots of warning logs in the console:
VIDEOJS: WARN: Using the tech directly can be dangerous. I hope you know what you're doing.
See https://github.com/videojs/video.js/issues/2617 for more info.
Which might seem confusing to users (and potentially bothersome). Should we consider removing that log message in video.js?
There was a problem hiding this comment.
At the very least, we should make a better landing page for that log. That currently points to the v5 related issue that recommends IWillNotUseThisInPlugins. We can create a page on the wiki (or on the videojs.com site) that says something along the lines of "be careful of doing stuff on the tech, pass in a value like true to suppress the warning".
I do think the warning is useful because a lot of people copy snippets of code without looking into the code. Hopefully, that warning is helping people think more about it.
|
Also, from a quick test, this LGTM as stuff like setting |
| @@ -237,15 +238,15 @@ parts of video.js: | |||
| // html5 for html hls | |||
| videojs(video, { | |||
| html5: { | |||
There was a problem hiding this comment.
This pull request also makes me wonder if we should support top level vhs options rather than just html5 -> vhs options
| }; | ||
|
|
||
| // HLS is a source handler, not a tech. Make sure attempts to use it | ||
| // as one do not cause exceptions. | ||
| Hls.canPlaySource = function() { | ||
| Vhs.canPlaySource = function() { | ||
| return videojs.log.warn('HLS is no longer a tech. Please remove it from ' + |
There was a problem hiding this comment.
Should this read HLS/VHS?
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Swap usage to `player.tech().vhs`
Description
Deprecate lingering references to hls from the old videojs-contrib-hls project, as well as dash. Ensure all references use "vhs" instead.
Remove unused utils/fixtures dir.
Requirements Checklist