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

Handle and return non-success status codes when loading files on web #286

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lunabunn
Copy link

@lunabunn lunabunn commented May 15, 2022

Successor of #284

  • Add WebAssetLoadingError(u16) to fs::Error a la AndroidAssetLoadingError
  • Call file loaded callback even on failure (fixes infinite hang when file fails to load)
  • Throw fs::Error::DownloadFailed on 404 (file not found), otherwise throw fs::Error::WebAssetLoadingError with the status code
  • Treat XMLHttpRequest errors as a 404 (file not found) and also throw fs::Error::DownloadFailed

Everything is tested, but I'm leaving this as a draft because:

  • This is a breaking change; should the gl.js version be bumped to 0.1.27?
  • This may return garbage if the server responds with an invalid status code (such as negative status codes or status codes over i16::MAX); I think this is fine, but do you want me to special-case this and throw DownloadFailed?

@lunabunn lunabunn marked this pull request as ready for review February 10, 2023 23:14
@lunabunn
Copy link
Author

@not-fl3 I already asked on Discord a few months ago, but can I please get your input on this? I have un-drafted it in case that's what was stopping you.

I see that on master you have a different solution for this, but the JS code there does not gracefully handle non-success error codes and is not very idiomatic overall. I think this is a nicer approach.

As stated in the PR itself, this is mostly ready to merge. Just waiting for review.

@not-fl3
Copy link
Owner

not-fl3 commented Feb 11, 2023

Thanks for pr! Will look closely into this after 0.4 merge!

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