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

Use fetch() in a way that works for file URLs #3071

Merged
merged 4 commits into from
Mar 11, 2025

Conversation

hughns
Copy link
Member

@hughns hughns commented Mar 10, 2025

fetch returns a response code of 0 when it successfully loads a file:// resource.

This means we can't just rely on response.ok.

Required for #2994

fetch returns a response code of 0 when it successfully loads a `file://` resource.

This means we can't just rely on `response.ok`.

Required for element-hq#2994
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

I think the condition

 !response.ok &&
        (!response.url || !response.url.startsWith("file:"))

Is un-intuitive enough so that is makes sense to move it into

/**
* Dedicated explanation why we need this for the file case here.
*/
isErrorNetworkOrFileResponse(response){
return  !response.ok &&
        (!response.url || !response.url.startsWith("file:"));
}

isErrorNetworkOrFileResponse can probably get a better name

Comment on lines 79 to 80
// if we are running embedded on file:// origin then we can't trust the response status
if (!response.ok && (!response.url || !response.url.startsWith("file:"))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure res.status === 404 || res.status === 0 implies response.ok === false (just double checking to make sure the or condition was actually unnecassary.)

Copy link
Contributor

Choose a reason for hiding this comment

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

why is it an && condition if we cannot trust the response status?
It (response.ok) is still guaranteed to be false in a failure case but it is also false if it is a success?

Copy link
Member Author

Choose a reason for hiding this comment

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

response.ok is defined as:

The ok read-only property of the Response interface contains a Boolean stating whether the response was successful (status in the range 200-299) or not.

fetch returns a status of 0 for file:// URLs.

I will put this into a function and clarify with comments.

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

Thanks with this change i now fully get what is happening.

@hughns hughns merged commit 1a692b9 into element-hq:livekit Mar 11, 2025
9 checks passed
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.

2 participants