-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix loading wasm in electron by falling back to XHR #12921
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
Fix loading wasm in electron by falling back to XHR #12921
Conversation
Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS. |
Thanks for the PR @0x53A - I agree we should fix this, but I think we may want a different approach. This regresses code size, which we try to avoid unless absolutely necessary. I think an approach that can work is the one suggested in #12203 That is, we could create an "electron shim" JS file that replaces |
It replaces the fetch with XmlHttpRequest IF the url is |
This reverts commit 53025c5.
Ok, then yes, I think that approach can be used here. That is, fetch would be wrapped around, so that it redirects a file URL to an XHR instead. Then that wrapping would be in a file that is only included in builds for electron, which would avoid a code size increase here. If that makes sense, then I can find time to set up the general infrastructure for adding such a file, if it's not obvious to you where to start for that. |
If I understood you correctly, this would replace ( If it is possible to detect electron at build time, wouldn't it be easier to wrap the else branch in I already wrapped the else in |
Oh, I meant to wrap it just inside the emscripten output. Not to modify the global state. (I assume we never want electron to fetch a file URL from our code?) But yes, if we can just ifdef by the build type then that is even better! Is the remaining code size increase in the diff (on that one test) unintentional then, and can be removed? |
No, the second one ( Only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks - yes, it makes sense for size to increase in that test, then.
src/preamble.js
Outdated
} | ||
|
||
// Otherwise, getBinary should be able to get it synchronously | ||
return Promise.resolve().then(getBinary); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does getBinary()
not work on Electron? If it allows sync xhrs on the main thread etc., then the addition above may not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getBinary
does a synchronous XHR, which is not allowed on the main thread: https://stackoverflow.com/questions/49577319/xmlhttprequest-error-synchronous-xmlhttprequest-on-the-main-thread-is-deprecate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, even on electron? I thought maybe since it has more APIs than the Web, that includes a way to synchronously get a file.
Thank you for the review. I finally had some more time for this and I think I've fixed all your comments. Regarding
I think there is a way to mark a protocol as "safe", which might even allow fetch on file, but the goal of this PR is to make WASM in Electron "just work" without every user having to code a workaround. |
Sorry for the late response - holidays etc... Looks good. I added an indentation fix now. Once tests pass, will merge. |
Test failures here look like existing issues on trunk, so looks safe to merge. |
Thank you! Hope you had some good holidays, and happy new year, etc ;D I'll ping the mono team after the next release. |
fixes mono/mono#20592 (which was my original issue)
should fix #11671
see electron/electron#3922
I did a quick test by editing dotnet.js in my electron output folder, and it seems to work, but would appreciate any feedback.