search

videojs / http-streaming

HLS, DASH, and future HTTP streaming protocols library for video.js
https://videojs-http-streaming.netlify.app/
Other
2.45k stars 418 forks source link

fix: wrap onwarn values in an object as expected by the playlist-loader #1428

Closed ianjaku closed 8 months ago

ianjaku commented 9 months ago

Description

Hey, apologies for not first opening an issue, but it's a very small fix. When debugging an issue in one of my projects I noticed the debug log: VIDEOJS: DEBUG: VHS: PlaylistLoader > m3u8-parser warn for [URL]: undefined.

This is caused by PlaylistLoader#parseManifest_ expecting onwarn to be called with an object that has a message parameter. In manifest.js#parseManifest the onwarn method is sometimes called with a string. It's also called with a message object inside the m3u8 parser.

Specific Changes proposed

We could either change onwarn to always return a string, or return a message object from inside manifest.js#parseManifest. In this fix I went with the latter as to not break any usage of parseManifest.

Please let me know if anything is unclear, I'll be happy to append the PR.

Requirements Checklist

welcome[bot] commented 9 months ago

πŸ’– Thanks for opening this pull request! πŸ’–

Things that will help get your PR across the finish line:

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codecov[bot] commented 9 months ago

Codecov Report

Merging #1428 (5c7edd6) into main (dd5e2af) will not change coverage. The diff coverage is 50.00%.

@@           Coverage Diff           @@
##             main    #1428   +/-   ##
=======================================
  Coverage   85.98%   85.98%           
=======================================
  Files          42       42           
  Lines       10428    10428           
  Branches     2412     2412           
=======================================
  Hits         8967     8967           
  Misses       1461     1461
Files Coverage Ξ”
src/manifest.js 93.57% <50.00%> (ΓΈ)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ianjaku commented 8 months ago

Is there a way to get this merged without writing additional tests?

The change is extremely simple, and there is no regression as this was untested behaviour before the change.

welcome[bot] commented 8 months ago

Congrats on merging your first pull request! πŸŽ‰πŸŽ‰πŸŽ‰