search

videojs / videojs-contrib-hls

HLS library for video.js
http://videojs.github.io/videojs-contrib-hls/
Other
2.84k stars 793 forks source link

simplify resolve-url #1328

Closed tjenkinson closed 5 years ago

tjenkinson commented 6 years ago

It is possible to remove some of the checks here, because they are already performed in the library.

If the relative url is actually absolute, it returns it here after running through one regex (which is cached), so it should also improve performance slightly

tjenkinson commented 6 years ago

Ah it does break a test

https://github.com/videojs/videojs-contrib-hls/blob/11987cc1cf987d1c17da86cc2e22f7a763b9174a/test/resolve-url.test.js#L14

Expected: "//a.com/b/cd/z.ts"
Actual: "http://a.com/b/cd/z.ts"

Maybe this is a problem, in which case just close this

gesinger commented 6 years ago

Thanks for the PR @tjenkinson !

re: the broken test case. Will the protocol that URLToolkit returns be the same as the page?

tjenkinson commented 6 years ago

The protocol will match the one that is the relative URL, or the base URL, or window.location.href

On 30 Jan 2018, at 00:18, Garrett Singer notifications@github.com<mailto:notifications@github.com> wrote:

Thanks for the PR @tjenkinsonhttps://github.com/tjenkinson !

re: the broken test case. Will the protocol that URLToolkit returns be the same as the page?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/videojs/videojs-contrib-hls/pull/1328#issuecomment-361421058, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ADG-WY_exlT8KN3S9SUyB1grMuNvrnHcks5tPlHRgaJpZM4Ru9yd.

forbesjo commented 5 years ago

Thank you for your PR but we will not be accepting new changes for this repo and will be archived very soon. I would advise you to open your PR against the next iteration of this project at https://github.com/videojs/http-streaming.