Skip to content

[file-packager] Use fetch() for loading file packages. #22016

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

Merged
merged 56 commits into from
Sep 20, 2024

Conversation

seanmorris
Copy link
Contributor

@seanmorris seanmorris commented May 30, 2024

This PR switches AJAX requests for file packages to be loaded with fetch() rather than XmlHttpRequest(). This allows service workers to make AJAX requests, since XmlHttpRequest is not available in service workers.

Module['setStatus'] is now called with more granularity during package downloads.

Previously, --embed-file was necessary to load file packages, which embeds the files into the binary and precludes caching and re-use of the package.

Partially fixes #22003, only applies to file packages.

@seanmorris seanmorris changed the title Use fetch() for loading file packages. Use fetch() for loading file packages. May 30, 2024
@sbc100 sbc100 changed the title Use fetch() for loading file packages. [file-packager] Use fetch() for loading file packages. Jun 3, 2024
};
</script>
{{{ SCRIPT }}}
</body>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than require special shell can you just rely on the fact that default reporting will try bot fetch and origFetch:

let doFetch = typeof origFetch != 'undefined' ? origFetch : fetch;

i.e. when you disable fetch you can cache the original fetch so that that browser reporting doesn't break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbc100 I'm not able to get the exception out of the browser without the custom shell. It just hangs with the exception listed as uncaught in the browser's console.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm modulo a few comments

@sbc100
Copy link
Collaborator

sbc100 commented Jun 9, 2024

@seanmorris do you have some time up update this PR? I'd love to see it landing if possible.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks for all the back and forth on this! I think this is ready to land now!

@seanmorris
Copy link
Contributor Author

Thanks for all the back and forth on this! I think this is ready to land now!

NICE!!!

auto-merge was automatically disabled September 20, 2024 02:14

Head branch was pushed to by a user without write access

@seanmorris seanmorris requested a review from sbc100 September 20, 2024 04:08
@sbc100 sbc100 merged commit 57aeff1 into emscripten-core:main Sep 20, 2024
28 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.

Can't preload files/shared libs in service worker due to lack of XHR [PR OPEN]
2 participants