Fix for loading wasm files under Node.js and in browser when files we… #5368

nazar-pc · 2017-07-07T14:46:18Z

I think this is related to #4542 and, probably, fixes it.

I have compiled a library with emcc supercop.c #files -o supercop.js -O2 --closure 1 -s WASM=1 and got 2 files in the root of the project: supercop.js and supercop.wasm, everything worked fine so far. However, when I published it to NPM effective path to WebAssembly file become node_modules/supercop.wasm/supercop.wasm and supercop.js file was unable to load it.

With this patch WebAssembly files that are located next to JavaScript files are loaded correctly both in Node.js and in browser.

I've run some tests and on the surface it looks fine, however, my 12-thread Core i7 is not enough to run all of the tests in any sane amount of time, so... Also I'm a bit terrified with number of tests being runned and do not quite understand how to add new tests. Hopefully someone can augment this PR with new tests.

… wasm loading from NPM module (upstream PR: emscripten-core/emscripten#5368)

kripken · 2017-07-07T17:30:31Z

Some questions:

Does the fetch API exist in node.js?
What is a concrete use case that shows the problem? (I'm not sure what publishing to npm entails, that you mentioned.) Looking for simple steps to reproduce the failure.

nazar-pc · 2017-07-07T17:40:12Z

fetch() is for browser only, so it doesn't matter whether Node supports it in this case.

Here is the project I've mentioned: https://github.com/nazar-pc/supercop.wasm
As you can see there is supercop.wasm in the root of the project, but when I install this package as dependency it is not in working directory anymore, but rather in node_modules/supercop.wasm subdirectory, thus supercop.wasm loading fails. With this patch it searches for supercop.wasm in the same directory as supercop.js rather than in current directory.

You can reproduce an issue by modifying supercop.wasm dependency in https://github.com/nazar-pc/bittorrent-dht-store-keypair/blob/browser-compatibility/package.json from ^3.0.1 to strictly 3.0.0 (which was compiled without fix from this PR) and run tests afterwards.

juj · 2017-07-10T13:10:00Z

This code now looks like it's going down the route that we wanted to avoid in PR #4942. That is, instead of having multiple locations in code that reference to __dirname or do complex statements such as document.currentScript.src.split('/').slice(0, -1).join('/');, it would be good to have a Module['scriptDirectory'] variable which does this only once, and all places of use would reference that variable. That would make handling this PR and #4942 look more or less identical. @nazar-pc : what do you think?

nazar-pc · 2017-07-10T13:24:22Z

I think having it in one place is a good idea overall, especially for some long ones like those needed in browser environment.
Also I think this PR is basically fixing another aspect of the same root issue as #4942, namely that the files are loaded from current working directory instead of the same directory as JavaScript file.

I also think we should possibly implement common path in either PR (or even yet another one) and then rebase against that.

Should I try to implement Module['scriptDirectory'] in this PR or in another one or will someone tackle this separately? I'd like to upstream fixes as soon as possible because relying on patched libraries and tooling is quite painful in my experience.

kripken · 2017-07-12T04:28:41Z

Can implement it in this PR, I think. Perhaps ask in the other PR if someone is working on it to be sure, but I don't think anyone is (last update there was over a month ago).

RReverser · 2017-07-21T19:51:10Z

Yeah, I got "distracted" from this area for the last month by other projects, so would be happy to see anyone implementing scriptDirectory suggestion.

nazar-pc · 2017-07-24T16:22:22Z

I think this can be called The ultimate fix.

I've found that over time Emscripten collected a few options that were fixing this exact issue for different types of files, namely:

memoryInitializerPrefixURL
pthreadMainPrefixURL
cdInitializerPrefixURL
filePackagePrefixURL

At least some of them (I've looked through some PRs that implemented those options) originated from the fact that people were moving files around and files stopped loading.

In fact, almost always you want to load all of the files from the same directory where .js file is placed simply because this is how building is done by default.

Now Module['locateFile'] will be defined in runtime if not specified before module loading. Newly introduced Module['scriptDirectory'] is used in default built-in Module['locateFile'] implementation.

This have one major implication: Module['locateFile'] is now always defined, so I've decreased its priority comparing to Module['filePackagePrefixURL'], Module['memoryInitializerPrefixURL'], Module['cdInitializerPrefixURL'] and Module['filePackagePrefixURL'], so that if any of these are specified, Module['scriptDirectory'] is not used for that particular use case.

I didn't deprecate Module['filePackagePrefixURL'], Module['memoryInitializerPrefixURL'], Module['cdInitializerPrefixURL'] or Module['filePackagePrefixURL'] yet, but chances are that even if you need to change them, they will all be the same, so effectively all you need is just new Module['scriptDirectory'](we can fallback to them during transition period).

Module documentation is also updated with new option (interestingly, not all of the mentioned options are mentioned there:))

This also makes #4942 unnecessary.

nazar-pc · 2017-07-24T16:23:15Z

Check this carefully, it might break some test with edge-cases, but shouldn't affect any real-world application.

kripken · 2017-07-24T16:29:23Z

I think I like this, nice. Seems like a good solution to use locateFile that way.

My one concern is the reordering of priorities with that and the prefixes. One option might be to add a warning if they "collide". Another is not to allow setting both prefixes and locateFile. Or, perhaps this is ok, as it's not that hard a problem to debug, it won't be silent/confusing errors?

For a change like this, we need input from @juj. He's on vacation right now, so this might need to wait a week or two.

nazar-pc · 2017-07-24T16:44:04Z

One option might be to add a warning if they "collide".

If they collide, more specific option wins, so Module['locateFile'] and Module['scriptDirectory'] are always used as last resort.

Another is not to allow setting both prefixes and locateFile.

Again, this is still supported, in this case Module['locateFile'] is ignored completely.

The only case when it might fail is if there is a test that deliberately defined both Module['locateFile'] and, for instance, Module['filePackagePrefixURL']. In this case Module['filePackagePrefixURL'] will win, while previously Module['locateFile'] was superior. But I'd argue new behavior is more correct actually.

Using latest Emscripten version (with emscripten-core/emscripten#5368 on top) that leads to even smaller build size.

nazar-pc · 2017-08-13T07:06:18Z

@juj, any objections here?

juj · 2017-08-14T16:33:33Z

ChangeLog.markdown

@@ -14,6 +14,9 @@ Current trunk code
    - Emscripten: https://github.com/kripken/emscripten/compare/1.37.13...incoming
    - Emscripten-LLVM: https://github.com/kripken/emscripten-fastcomp/compare/1.37.13...incoming
    - Emscripten-Clang: https://github.com/kripken/emscripten-fastcomp-clang/compare/1.37.13...incoming
+ - Added new html setting Module['scriptDirectory'] that allows customizing the URL where .wasm, .mem and some other files are located (defaults to the same location as .js file)
+ - If Module['locateFile'] is not specified explicitly it will be defined automatically using Module['scriptDirectory']
+ - Module['locateFile'] priority comparing to other options decreased since it is now always defined


The format that is used in the ChangeLog is to first have the changes, and then at the end of each changes to have the block

- To see a list of commits in the active development branch 'incoming', which have not yet been packaged in a release, see - Emscripten: https://github.com/kripken/emscripten/compare/1.37.13...incoming - Emscripten-LLVM: https://github.com/kripken/emscripten-fastcomp/compare/1.37.13...incoming - Emscripten-Clang: https://github.com/kripken/emscripten-fastcomp-clang/compare/1.37.13...incoming

so these would go four lines up.

juj · 2017-08-14T16:52:35Z

Demoting Module['locateFile'] to have different priority over the other methods would be a breaking change. Also the semantics of Module['locateFile'] will change from being a user defineable override to having new semantics that one will need to adhere to. For example, I have instructed developers in the past to write these types of locateFile functions:

Module['locateFile'] = function(url) { return url + '.gz'; }

to set up gzip compression for all assets, and this new Module['scriptDirectory'] would be ignored on all these uses. We should not change the semantics after shipping, unless there is a good reason. This does not seem like there is a need to do so?

I didn't deprecate Module['filePackagePrefixURL'], Module['memoryInitializerPrefixURL'], Module['cdInitializerPrefixURL'] or Module['filePackagePrefixURL'] yet

I'm confused that you mention about deprecating these, but still changed to bump the priority of these to take over Module['locateFile']?

My understanding was that the consensus was that we don't ever want to load the .mem, .wasm etc. files from node.js's CWD, but always relative to the location of the main .js file. If this was solved according to https://github.com/kripken/emscripten/pull/4942/files#r113408121, then the semantics or priority of Module['locateFile'] would not change, and relative loading would automatically get fixed for node.js developers as well who use Module['locateFile']. That seems superior to me, since it preserves existing functionality and fixes the relative path loading thing only that is the root bug?

nazar-pc · 2017-08-14T18:01:10Z

Here is what was before:

            var memoryInitializer = '%s';
            if (typeof Module['locateFile'] === 'function') {
              memoryInitializer = Module['locateFile'](memoryInitializer);
            } else if (Module['memoryInitializerPrefixURL']) {
              memoryInitializer = Module['memoryInitializerPrefixURL'] + memoryInitializer;
            }

And here is what will be after proposed changes:

            var memoryInitializer = '%s';
            if (Module['memoryInitializerPrefixURL']) {
              memoryInitializer = Module['memoryInitializerPrefixURL'] + memoryInitializer;
            } else {
              memoryInitializer = Module['locateFile'](memoryInitializer);
            }

This is what I mean by changed priority. Since Module['locateFile'] is now always defined, we need to lower its priority. We could introduce another function private to Emscripten and call something like var memoryInitializer = anotherFunction('%s'); seems redundant.

Yes, this is a breaking change for some edge cases. But it doesn't break the code you've instructed other developers with, unless they're also defining other options like memoryInitializerPrefixURL at the same time (which is extremely unlikely because it doesn't make any sense).

kripken · 2017-08-14T19:41:58Z

For the edge cases, could we show an error if they occur? I don't think we expect memoryInitializerPrefixURL to be used with locateFile, so perhaps we could just disallow that with a clear error (instead of a potentially surprising behavior change)?

nazar-pc · 2017-08-14T19:53:31Z

I don't think we expect memoryInitializerPrefixURL to be used with locateFile

Taking into account that with current implementation locateFile is always defined (by user or implicitly), this makes memoryInitializerPrefixURL obsolete?

If there is no use case where, say, filePackagePrefixURL is different from memoryInitializerPrefixURL, cdInitializerPrefixURL or filePackagePrefixURL, then they are all obsolete in favor of generic scriptDirectory.

nazar-pc · 2017-08-14T19:54:59Z

I think if we encounter any of those *PrefixURL we can treat all of them as scriptDirectory aliases now.

kripken · 2017-08-14T19:55:58Z

Sorry, I wasn't clear. I meant that if the user provides both memoryInitializerPrefixURL and locateFile, we should give an error. But the user can still provide either one (but just one). (That we implicitly generate a locateFile is an internal detail.)

nazar-pc · 2017-08-14T19:58:53Z

Makes sense, will add corresponding checks.

What about deprecating *PrefixURL?

juj · 2017-08-16T15:37:56Z

Since Module['locateFile'] is now always defined, we need to lower its priority.

I don't think it's good to always require having Module['locateFile'] defined. It would be better to move to a direction where we have fewer mandatory properties defined on Module, rather than more. The needed path redirection can be implemented without requiring users to create a suitable Module['locateFile'].

In particular, the root problem here would still remain, that if user implements their own Module['locateFile'], if they return a relative URL from this function, it will still be treated differently in Node.js and in the browser. In Node.js with the proposed change, relative URLs returned by Module['locateFile'] will still be treated relative to Node.js CWD, and in the browser, relative URLs will be treated relative to the .html file. In this form, we are placing an added requirement to developers that they should always return an absolute URL from Module['locateFile'] and never a relative one, if they want to stay consistent with respect to Node.js and the browser.

Yes, this is a breaking change for some edge cases. But it doesn't break the code you've instructed other developers with, unless they're also defining other options like memoryInitializerPrefixURL at the same time

This breaks in emunittest suite, where I implement Module.locateFile to existing builds that developers provide to override hosting of built projects, in the form of Module.locateFile: function(url) { return cdnRoot + url + '.gz'; }. Unity uses both filePackagePrefixURL and memoryInitializerPrefixURL with either Release/ or Compressed/ by default. This change will mean that if one wants to specify Module.locateFile to configure paths of all deployed items on a CDN, they'll need to be aware of and delete all the other properties, if they might exist.

What about deprecating *PrefixURL?

I don't think there is a need to deprecate to fix the issue. These are features that developers are using, and they are tested and there is no inherent problem with them, except than a couple of added bytes. I think we should focus to fix the root problem that relative URLs for these specific files should be treated relative to the script and not the CWD.

I have posted PR #5484 as a suggestion to fix this. It changes the meaning of relative URLs from these built-in functions to be interpreted with respect to the main .js file on Node.js, but keep everything else the way it is so it will not break anything.

juj · 2017-08-16T15:40:12Z

src/shell.js

+    } else if (ENVIRONMENT_IS_WEB) {
+      Module['scriptDirectory'] = document.currentScript.src.split('/').slice(0, -1).join('/') + '/';
+    } else if (ENVIRONMENT_IS_WORKER) {
+      Module['scriptDirectory'] = self.location.href.split('/').slice(0, -1).join('/') + '/';


On the web and in web workers, XHRs are already treated relative to the current location by default, we don't need to explicitly reinforce that.

http://test/xhr.html:

<!doctype html> <script src="/xhr/test.js"></script>

/xhr/test.js:

var xhr = new XMLHttpRequest(); xhr.open('GET', 'test2', true); xhr.send(null);

The call is made to http://test/test2 instead of http://test/xhr/test2.

Also there is <base> tag that have influence on relative paths. I think being explicit here is a good thing.

I think if someone uses <base>, they explicitly want a different base path for any resources and that shouldn't get broken.

I've just clarified that the statement that was made is not true and there is a value in determining an absolute path here.

nazar-pc · 2017-08-16T19:17:54Z

I don't think it's good to always require having Module['locateFile'] defined.

It is not required to be specified explicitly by user: https://github.com/kripken/emscripten/pull/5368/files#diff-8f35bc569987bab9a3691d986d90e75fR74

if they return a relative URL from this function, it will still be treated differently in Node.js and in the browser

This is expected, user have to identify which environment is used and behave accordingly.

In Node.js with the proposed change, relative URLs returned by Module['locateFile'] will still be treated relative to Node.js CWD

And why is this a problem? With proposed change most of the time user wouldn't need to touch this option in the first place. And if he/she ever does, I don't see any problem with specifying absolute path.

and in the browser, relative URLs will be treated relative to the .html file.

In modern applications there might be no single .html file and relative paths are more complicated that that (for instance, they also depend on <base> tag).

In this form, we are placing an added requirement to developers that they should always return an absolute URL from Module['locateFile'] and never a relative one, if they want to stay consistent with respect to Node.js and the browser.

This is exactly the same requirement as before. On server especially user should always infer absolute path (using __dirname or otherwise) just to be sure that correct path is always used.

I don't think there is a need to deprecate to fix the issue. These are features that developers are using, and they are tested and there is no inherent problem with them, except than a couple of added bytes.

True, but they are not useful either once there is a universal Module['scriptDirectory']. I don't think it is healthy to have 4 prefix variables that literally do the same thing and one more on top of them to act like prefix to already prefixed paths (here I'm talking about #5484). Deprecating those redundant options in advance would give developers plenty of time to migrate, I have nothing against keeping them till next major version.

nazar-pc · 2017-08-16T19:18:54Z

I'd like to make this a separate comment to clarify about relative paths:

Returning relative paths from Module['locateFile'] is not a good idea in general (and with proposed change behavior of Module['locateFile'] stays exactly the same as before, which is good for backward compatibility)
Even when relative path is returned, both in Node.js and in browser it will behave exactly like user expects under any other circumstance (e.g. filesystem operations and XHR requests): relatively to CWD, relatively to the root of the website or relatively to the <base href>
Making relative paths relative to something like script location is unexpected and confusing

…ishing to NPM

Added `ready()` method. New keywords added. Fresh build still uses emscripten-core/emscripten#5368

…tion

nazar-pc

Yes, test_glgears_proxy_jstarget works both in Firefox Stable and Nightly (at least here on Linux)

nazar-pc · 2018-07-16T18:54:00Z

emcc.py

+};
+%(EXPORT_NAME)s = %(EXPORT_NAME)s.bind({
+  _currentScript: typeof document !== 'undefined' ? document.currentScript : undefined
+})%(instantiate)s;


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.

nazar-pc · 2018-07-16T18:56:48Z

emcc.py

-          } else if (Module['memoryInitializerPrefixURL']) {
-            memoryInitializer = Module['memoryInitializerPrefixURL'] + memoryInitializer;
-          }
+          memoryInitializer = Module['locateFile'] ? Module['locateFile'](memoryInitializer, '') : memoryInitializer;


I'm not very familiar with this part of the code base and it turns out that in tests Module is not eventual module, but rather an object that real Module will extend. Hence there is no Module._locateFile at this point yet.

nazar-pc · 2018-07-16T19:13:50Z

site/source/docs/api_reference/module.rst

 .. js:attribute:: Module.locateFile

-	If set, this method will be called when the runtime needs to load a file, such as a ``.wasm`` WebAssembly file, ``.mem`` memory init file, or a file generated by the file packager. The function receives the URL, and should return the actual URL. This lets you host file packages or the ``.mem`` file etc. on a different location than the current directory (which is the default expectation), for example if you want to host them on a CDN. Note that ``locateFile`` is sort of a generalization of ``Module.filePackagePrefixURL``.
+	If set, this method will be called when the runtime needs to load a file, such as a ``.wasm`` WebAssembly file, ``.mem`` memory init file, or a file generated by the file packager. The function receives the relative path to the file as configured in build process and a prefix (path to JavaScript file), and should return the actual URL. This lets you host file packages or the ``.mem`` file etc. on a different location than the directory of JavaScript file (which is the default expectation), for example if you want to host them on a CDN.


Yes, there are some places where Module['locateFile'] is called, but Module itself was not initialized yet. I'd like to see this re-designed this somehow, but not sure if it is possible.

I've added a note in latest commits about these cases to the degree I understand them.

nazar-pc · 2018-07-16T19:29:58Z

src/shell.js

+#endif
+// `/` should be present at the end if `scriptDirectory` is not empty
+var scriptDirectory = '';
+#if !SUPPORT_BASE64_EMBEDDING


I didn't fully understand how it works when SUPPORT_BASE64_EMBEDDING landed, but it seems like there were some guards added in relevant places, I'll try to remove this check and see if any tests fail.

nazar-pc · 2018-07-16T19:32:11Z

src/shell.js

+#if !SUPPORT_BASE64_EMBEDDING
+#if ENVIRONMENT_MAY_BE_NODE
+if (ENVIRONMENT_IS_NODE) {
+  scriptDirectory = __dirname + '/';


It can be, but I'd prefer to keep everything related to Module._locateFile next to each other, this way it is easier to follow execution flow.

nazar-pc · 2018-07-16T19:35:55Z

src/shell.js

+}
+#endif
+#endif // SUPPORT_BASE64_EMBEDDING
+Module._locateFile = function (path) {


I think we can, applied this change in one of latest commits.

nazar-pc · 2018-07-16T21:49:02Z

Now different test test_sdl2_image_prepare failed instead, looks like some flakiness on Circle CI's side. Works fine for me locally.

paulshapiro · 2018-07-16T21:57:12Z

@nazar-pc testing under Electron in the renderer (web) process, I don't believe it thinks it's node.js, so it looks for the wasm file in the dir that holds the index.html file (and fails to find it). Any advice? Hoping to avoid having to set wasmBinary ... especially as I actually don't have any idea how I'd do that in a require() type call as we have without some sort of platform-specific post-js...

nazar-pc · 2018-07-16T22:06:36Z

@paulshapiro, if you're building module just for Electron, then try ENVIRONMENT from https://kripken.github.io/emscripten-site/docs/optimizing/Optimizing-Code.html#miscellaneous-code-size-tips or otherwise this is one of the use cases where you actually need to define custom Module.locateFile.

paulshapiro · 2018-07-16T22:16:04Z

@nazar-pc that's just the thing - our lib is consumed by people using all kinds of different platforms - web, Node.JS, React, etc. We've had a working emscripten build for ~4 yrs but it's getting modernized now. I was thinking this is the use-case for locateFile but my concern is that given there's a require() (MODULARIZE on), I can't specify parameters (the parent dir) to the module load beforehand. Feel like I'm missing something :)

nazar-pc · 2018-07-17T07:17:54Z

There was Module['ENVIRONMENT'] before that allowed you to force specific environment overriding autodetection. I'd like to avoid introducing workarounds for Electron specifically.
In the mean time it might be possible to do by specifying custom library.

curiousdannii · 2018-07-17T07:36:40Z

@nazar-pc I don't know if you'd want to include it in this PR, but the platform detection code in shell.js really needs an overhaul. Electron should either be it's own platform, or Node should be tested before the browser, or Node shouldn't do && !ENVIRONMENT_IS_WEB. I think testing for Node before browser is probably safest, but I haven't looked at all the implications.

paulshapiro · 2018-07-17T13:08:02Z

@nazar-pc @curiousdannii fwiw here's what I'd been using

var ENVIRONMENT_IS_WEB=typeof window==="object";

var ENVIRONMENT_IS_WORKER=typeof importScripts==="function";

var ENVIRONMENT_IS_NODE=typeof process==="object"&&process.browser !== true&&typeof require==="function" && ENVIRONMENT_IS_WORKER == false; // we want this to be true for Electron but not for a WebView nor e.g. Cordova (latter not checked here)

var ENVIRONMENT_IS_SHELL=!ENVIRONMENT_IS_WEB&&!ENVIRONMENT_IS_NODE&&!ENVIRONMENT_IS_WORKER;

kripken · 2018-07-17T20:54:00Z

About Electron, #6803 has a proposal for users replacing the "core JS" which is what detects and sets up the environment. That could provide a way to define a custom Electron core JS file.

In the shorter term, if Electron is currently blocked, we could add an option to do something like the old Module['ENVIRONMENT'] runtime detection, but let's open a separate issue for that discussion please.

paulshapiro · 2018-07-17T21:20:28Z

In the shorter term, if Electron is currently blocked

@kripken understood, thanks. The reason I'm asking about it in this venue is that if the parent directory behavior for file location is contingent upon some set of environment detection functions which are internal to Module, then they're only being used indirectly to trigger certain expected loading functionality in emscripten. That loading behavior may not actually be well defined anymore given that Node can be run in so many contexts. Reproducing the same loading behavior would ostensibly be done by mapping the actual environment to the one which will have the desired loading behavior, but that causes complexity because the environment might not truly be only node. For that reason it leaves me to wonder if feature detection is not a more fitting method in the sense that a desired parent directory or locate file function could be passed in as desired, relieving emscripten of the burden to match environments.

nazar-pc · 2018-07-18T19:54:38Z

@paulshapiro, I think these kind of issues should be discussed separately. You can get current behavior of Emscripten after this PR by simply using Module.locateFile = function (path) {return path;}.
So at least it doesn't get worse than before. While I'd like to see Electron and other environments supported, let's merge at least something eventually.

If there are other issues to be fixed here, let me know and I'll be able to resolve them on weekend, otherwise I think PR is in pretty good shape.

nazar-pc

Will address those points on weekend

nazar-pc · 2018-07-18T20:19:41Z

emcc.py

+};
+%(EXPORT_NAME)s = %(EXPORT_NAME)s.bind({
+  _currentScript: typeof document !== 'undefined' ? document.currentScript : undefined
+})%(instantiate)s;


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's document.currentScript from the outside.

I'm more inclined to write more tests here rather than dealing with it every time I use Emscripten.

nazar-pc · 2018-07-18T20:20:22Z

src/shell.js

+#if !SUPPORT_BASE64_EMBEDDING
+#if ENVIRONMENT_MAY_BE_NODE
+if (ENVIRONMENT_IS_NODE) {
+  scriptDirectory = __dirname + '/';


OK, will refactor it.

…are moved somewhere and `-s MODULARIZE=1` is used at the same time

nazar-pc · 2018-07-22T16:56:00Z

@kripken, addressed your last comments in 2 latest commits.

kripken · 2018-07-22T21:30:31Z

Great, thanks @nazar-pc!

Let's merge this in now. I have a few minor changes, mainly in docs, that I'll do in a followup PR to this one.

I just had one question about the code (that can also wait for a followup PR, if we need one) - I see you handle a blob: URL for the web but not worker case - is there a reason not to also check in the worker case as well? It would also be nice to have a test for the blob URL case.

Thanks again for your patience in this PR - sorry it took this long to get merged.

nazar-pc · 2018-07-22T23:52:57Z

I believe there is already a test case for blob: that was failing with earlier iterations of this PR, so I've added corresponding check.
It is hard to imagine why something like that would be useful in worker though, but in case someone have a legitimate use case it is always possible to add one more check there.

* whitespace * docs * clarify MODULARIZE usage of _currentScript, and ifdef it

RReverser · 2018-07-23T15:45:25Z

Omg, thank you @nazar-pc and @kripken. I lost hope that this would be merged a long time ago, happy to be proved wrong.

bvibber · 2018-07-25T04:13:09Z

Hmm, that 'bind' call for setting _currentSource doesn't appear to do anything. With 1.38.10 with this patch, ogv.js's demuxer modules are broken because this._currentSource is null when checked. #6903

bvibber · 2018-07-25T19:15:40Z

Ok, that was breaking because I was calling the module function with "new", which replaces the "this" objected provided by the "bind" with a new one with no properties on it.

Fix is to follow current documentation and not use "new" with the module function (it's not a constructor, but an .... initializer?)

nazar-pc added a commit to nazar-pc/supercop.wasm that referenced this pull request Jul 7, 2017

Better compression is used, recompiled with patched Emscripten to fix…

954919f

… wasm loading from NPM module (upstream PR: emscripten-core/emscripten#5368)

nazar-pc added a commit to nazar-pc/supercop.wasm that referenced this pull request Aug 9, 2017

Simplified integration by using source functions directly.

3cbd6be

Using latest Emscripten version (with emscripten-core/emscripten#5368 on top) that leads to even smaller build size.

nazar-pc mentioned this pull request Aug 9, 2017

Web worker files in other folders #5452

Closed

juj reviewed Aug 14, 2017

View reviewed changes

juj mentioned this pull request Aug 16, 2017

fix_relative_file_loads_node_js #5484

Closed

juj reviewed Aug 16, 2017

View reviewed changes

nazar-pc added a commit to nazar-pc/noise-c.wasm that referenced this pull request Sep 6, 2017

Test build, would require emscripten-core/emscripten#5368 before publ…

70924a3

…ishing to NPM

nazar-pc added a commit to nazar-pc/supercop.wasm that referenced this pull request Sep 6, 2017

Better WebAssembly size optimization.

f3ef167

Added `ready()` method. New keywords added. Fresh build still uses emscripten-core/emscripten#5368

nazar-pc mentioned this pull request Sep 17, 2017

Support node env override in Electron renderer process #5577

Closed

nazar-pc added 2 commits July 16, 2018 22:28

Try removing potentially unnecessary SUPPORT_BASE64_EMBEDDING condi…

4bd0e76

…tion

Refactor Module._locateFile method to locateFile function

ff119a9

nazar-pc commented Jul 16, 2018

View reviewed changes

nazar-pc commented Jul 18, 2018

View reviewed changes

nazar-pc added 2 commits July 22, 2018 19:11

Add test case for _currentScript hack that is necessary when files …

d1354f1

…are moved somewhere and `-s MODULARIZE=1` is used at the same time

Refactor as suggested in review comments

7a3ec9c

kripken merged commit d1f303a into emscripten-core:incoming Jul 22, 2018

nazar-pc deleted the fix-wasm-loading-for-moved-files branch July 22, 2018 23:49

kripken added a commit that referenced this pull request Jul 23, 2018

Minor followups to #5368 (#6888)

be5f44a

* whitespace * docs * clarify MODULARIZE usage of _currentScript, and ifdef it

kripken mentioned this pull request Jul 23, 2018

Generalize blob url handling in scriptDirectory computation to also handle workers #6894

Merged

paulshapiro mentioned this pull request Jul 23, 2018

Memory Leak Problem mymonero/mymonero-core-js#8

Closed

This was referenced Sep 17, 2018

Search for .mem in same directory as Node script #4942

Closed

Use import.meta.url in EXPORT_ES6 mode #7137

Closed

benbro mentioned this pull request May 30, 2021

Wrong path for tflite.wasm Volcomix/virtual-background#16

Closed

Fix for loading wasm files under Node.js and in browser when files we… #5368

Fix for loading wasm files under Node.js and in browser when files we… #5368

Conversation

nazar-pc commented Jul 7, 2017

kripken commented Jul 7, 2017

nazar-pc commented Jul 7, 2017

juj commented Jul 10, 2017

nazar-pc commented Jul 10, 2017

kripken commented Jul 12, 2017

RReverser commented Jul 21, 2017

nazar-pc commented Jul 24, 2017

nazar-pc commented Jul 24, 2017

kripken commented Jul 24, 2017

nazar-pc commented Jul 24, 2017 • edited Loading

nazar-pc commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juj commented Aug 14, 2017

nazar-pc commented Aug 14, 2017

kripken commented Aug 14, 2017

nazar-pc commented Aug 14, 2017

nazar-pc commented Aug 14, 2017

kripken commented Aug 14, 2017 • edited Loading

nazar-pc commented Aug 14, 2017

juj commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc commented Aug 16, 2017

nazar-pc commented Aug 16, 2017

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc commented Jul 16, 2018

paulshapiro commented Jul 16, 2018 • edited Loading

nazar-pc commented Jul 16, 2018

paulshapiro commented Jul 16, 2018 • edited Loading

nazar-pc commented Jul 17, 2018

curiousdannii commented Jul 17, 2018 • edited Loading

paulshapiro commented Jul 17, 2018 • edited Loading

kripken commented Jul 17, 2018

paulshapiro commented Jul 17, 2018

nazar-pc commented Jul 18, 2018

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc commented Jul 22, 2018

kripken commented Jul 22, 2018

nazar-pc commented Jul 22, 2018

RReverser commented Jul 23, 2018

bvibber commented Jul 25, 2018

bvibber commented Jul 25, 2018

nazar-pc commented Jul 24, 2017 •

edited

Loading

kripken commented Aug 14, 2017 •

edited

Loading

paulshapiro commented Jul 16, 2018 •

edited

Loading

paulshapiro commented Jul 16, 2018 •

edited

Loading

curiousdannii commented Jul 17, 2018 •

edited

Loading

paulshapiro commented Jul 17, 2018 •

edited

Loading