-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 for loading wasm files under Node.js and in browser when files we… #5368
Changes from all commits
907dc04
deb8520
011fe85
ffb2f56
5bf91f9
027380d
0ee1e2f
a1dd1db
82ef1e0
25c65b9
07ed0bc
4099e28
f859edf
1bfa6b0
4ca757c
0e3d19e
d0deafd
d221d2e
11ae2e0
7d453dd
70a1abc
c84f4f0
96f56a1
63bf3fc
5f3601b
59eabb3
4bd0e76
ff119a9
d1354f1
7a3ec9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Original file line | Diff line number | Diff line change |
---|---|---|---|
|
@@ -2598,7 +2598,10 @@ def modularize(): | ||
%(src)s | %(src)s | ||
|
|
||
return %(EXPORT_NAME)s; | return %(EXPORT_NAME)s; | ||
}%(instantiate)s; | }; | ||
%(EXPORT_NAME)s = %(EXPORT_NAME)s.bind({ | |||
_currentScript: typeof document !== 'undefined' ? document.currentScript : undefined | |||
})%(instantiate)s; | |||
''' % { | ''' % { | ||
'EXPORT_NAME': shared.Settings.EXPORT_NAME, | 'EXPORT_NAME': shared.Settings.EXPORT_NAME, | ||
'src': src, | 'src': src, | ||
|
@@ -2704,11 +2707,7 @@ def generate_html(target, options, js_target, target_basename, | ||
script.un_src() | script.un_src() | ||
script.inline = (''' | script.inline = (''' | ||
var memoryInitializer = '%s'; | var memoryInitializer = '%s'; | ||
if (typeof Module['locateFile'] === 'function') { | memoryInitializer = Module['locateFile'] ? Module['locateFile'](memoryInitializer, '') : memoryInitializer; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why does this not call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not very familiar with this part of the code base and it turns out that in tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, right, this is code in the HTML, which declares |
|||
memoryInitializer = Module['locateFile'](memoryInitializer); | |||
} else if (Module['memoryInitializerPrefixURL']) { | |||
memoryInitializer = Module['memoryInitializerPrefixURL'] + memoryInitializer; | |||
} | |||
Module['memoryInitializerRequestURL'] = memoryInitializer; | Module['memoryInitializerRequestURL'] = memoryInitializer; | ||
var meminitXHR = Module['memoryInitializerRequest'] = new XMLHttpRequest(); | var meminitXHR = Module['memoryInitializerRequest'] = new XMLHttpRequest(); | ||
meminitXHR.open('GET', memoryInitializer, true); | meminitXHR.open('GET', memoryInitializer, true); | ||
|
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.
did you find if there's a test that this is necessary to get passing?
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.
No, there is no corresponding test unfortunately. I did this after using earlier version of this PR in my projects. Not a perfect or bullet-proof solution, but it works more often than without it.
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.
What do you think about leaving this up to the user, instead? We could document that
document.currentScript
is used, and so if the page will modify that, the user should implementlocateFile
in order to get things working properly.Alternatively, we could make more of an effort here: write up tests that ensure the right behavior, and document what cases exactly we handle. Basically, it's a "best-effort" without clear guidelines and tests that I'd prefer to avoid.
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.
Well, my idea was to make it just work by default and it does. I'd like to avoid user to require doing anything special just to get this basic thing working, re-implementing
locateFile
would be a big pain, especially since it would be necessary to get script'sdocument.currentScript
from the outside.I'm more inclined to write more tests here rather than dealing with it every time I use Emscripten.