-
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
fix(player): techGet is undefined #8256
fix(player): techGet is undefined #8256
Conversation
I'm not sure why this causes the test failure... |
@mister-ben I don't know why it causes the tests to fail, especially since locally I didn't have any issue. I was tempted to delete the tests 😅, but it seemed important to add the test coverage. I wonder if it's a timer issue, a clock.tick missing, but it still seems strange. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of amazing this hasn't been encountered yet.
Approved before I noticed its test was failing... change looks good, though.
@mister-ben, @misteroneill thank you for taking the time to review this PR. The test added in this PR seems to produce a timeout in Error message
So there are two options, delete the test case if it is not useful or move it to Finally, while writing the unit test for this fix I came across a behavior that I'm not quite sure I understand. It's all in the Demonstrationplayer.src('https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8');
player.cache_.src;
// "https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8"
player.cache_.source;
// { src: "https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8", type: "application/x-mpegURL" }
player.cache_.sources;
// [{ src: "https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8", type: "application/x-mpegURL" }]
player.load();
player.cache_.src;
// "blob:http://localhost:9999/a1dc9574-404a-47a2-9d00-1ff7b0949abb"
player.cache_.source
// { src: "blob:http://localhost:9999/a1dc9574-404a-47a2-9d00-1ff7b0949abb", type: "" }
player.cache_.sources
//[{ src: "blob:http://localhost:9999/a1dc9574-404a-47a2-9d00-1ff7b0949abb", type: "" }] I couldn't tell if this is the expected behavior, however it creates a playback issue on Dash and HLS content, see PR #8274 |
Hey! We've detected some video files in a comment on this issue. If you'd like to permanently archive these videos and tie them to this project, a maintainer of the project can reply to this issue with the following commands:
|
Yeah, calling load() with VHS enabled is known to be broken right now. I've long thought that we should forward the call to the tech and then the source handler, but your PR seems like a good workaround. |
cf39dee
to
c25cbdd
Compare
Codecov Report
@@ Coverage Diff @@
## main #8256 +/- ##
=========================================
+ Coverage 0 82.40% +82.40%
=========================================
Files 0 112 +112
Lines 0 7483 +7483
Branches 0 1804 +1804
=========================================
+ Hits 0 6166 +6166
- Misses 0 1317 +1317
... and 111 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@mister-ben, @misteroneill I fixed the unit test, I forgot to put a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
Description
This PR fixes #8255 the call to
techGet_
.Specific Changes proposed
techGet
totechGet_
Requirements Checklist