Skip to content
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

fixed issue with manifest not working in safari #6

Merged
merged 4 commits into from
Apr 12, 2022

Conversation

Sgtfishtank
Copy link
Contributor

No description provided.

@birme
Copy link
Contributor

birme commented Apr 5, 2022

Aha, so the issue was actually the content type. Have you tested to set the correct Content-Type ?
It should not be mandatory to have .m3u8 as suffix

@@ -43,10 +43,10 @@ export const handler: ALBHandler = async (event: ALBEvent): Promise<ALBResult> =
event.queryStringParameters = refineALBEventQuery(event.queryStringParameters);
let response: ALBResult;
try {
if (event.path.match(/manifests\/hls\/proxy-master$/) && event.httpMethod === "GET") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kan vi göra en
// nånstans
const HLS_MANIFEST_PROXY_MASTER = /manife.../
const HLS_MANIFEST_PROXY_MEDIA = ...

const { event, path} = event
if !event.httpMethod.GET returnSomeError()

switch(event.path){
case "kolla HLS_MANIFEST_PROXY_MASTER direkt eller via nån func":
...
}

#Diskutera

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. jo ok, om man ska skippa regex med match, får vi först stippa bort /api/v2/ då event.path kommer att innehålla eg /api/v2/manifests/hls/proxy-master

om inte vi vill köra med hela grejen...
const HLS_PROXY_MASTER_PATH="/api/v2/manifests/hls/proxy-master"

så det kan absolut gå med switch/case.

dock väntar vi oss GET eller OPTIONS http metoder, så det får bli

if event.httpMethod !==GET || event.httpMethod !== OPTIONS returnSomeError()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okok, lite tl;dr jag såg inte options :D

Copy link
Contributor

@Thurfjell Thurfjell Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man kanske kan göra en
GET:
--stuff..
--return 2xx
OPTIONS:
--stuff...
--return 2xx

return 500 fail

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jag tänkte mer att man kanske kan konstantifiera regexen.
stuff.match(MY_SPECIFIC_REGEX)
Ingen big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jag håller med om att det blir bättre och enklare att läsare gör dina föreslagna ändringar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

För att göra det lättare om ska ändra på ett route, och så att men slipper ändra på två ställen, så skulle vi nog behöva en constants.ts fil där vi kan definera route constanter som kan återanvända i alla routes.ts (master/media/segments/dash) och lambda.ts

@Sgtfishtank Sgtfishtank requested a review from Thurfjell April 8, 2022 12:11
Copy link
Contributor

@Nfrederiksen Nfrederiksen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Sgtfishtank Sgtfishtank merged commit 637b7d5 into main Apr 12, 2022
@Sgtfishtank Sgtfishtank deleted the Proxy-manifests-not-working-in-native-Safari-Player branch April 12, 2022 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants