search

Touffy / client-zip

A client-side streaming ZIP generator
MIT License
353 stars 23 forks source link

[BUG] downloadZip is not a function when running the build script #47

Open wlinna opened 2 years ago

wlinna commented 2 years ago

Describe the bug The unmodified worker demo doesn't start the download, it only shows downloadZip is not a function error when trying to download.

The issue is that the worker.js produced by the build script is an ES-module, and needs hand-editing to turn it into an IIFE.

To Reproduce Steps to reproduce the behavior:

  1. Clone the repo (I'm on commit c740db56fe586bf834cd1ad0f08ae01eb957f90c)
  2. Run npm run start
  3. Start a web-server at the root dir of the repo (in my case python3 -m http.server 8000)
  4. Open http://localhost:8000/demo/worker.html in your browser
  5. Add https://raw.githubusercontent.com/Touffy/client-zip/master/CHANGES.md to the list of URLs
  6. Click Download all that in one ZIP
  7. Observe the downloadZip is not a function

Expected behavior Start the download (I haven't actually seen how the demo should work)

Workaround Instead of compiling the scripts yourself, use a prebuilt worker.js from npm, GitHub Releases or remove module wrapper manually.

Desktop (please complete the following information):

Additional context npm --version 8.15 node --version v16.17.0 python3 --version 3.10.4

Touffy commented 2 years ago

I am not familiar with python's http server, but I don't expect it adds Content-Security-Policy headers. You don't have any trouble with instantiating the WebAssembly module in the Service Worker ? If that fails, downloadZip won't be defined.

Touffy commented 2 years ago

More importantly, does this happen only on your local hosting of client-zip and not on mine ?

wlinna commented 2 years ago

It's difficult to say whether there are problems with the instantiation. There are no errors, warnings or any other messages in the console, and a WebAssembly module actually appears in the sources tab of the Dev Tools.

Also, downloadZip IS defined, it just isn't a function. This is what the debugger shows for it:

default:  (...)
__esModule: 1

I'm not sure about the CSP-headers. I can take a look at them next.

wlinna commented 2 years ago

More importantly, does this happen only on your local hosting of client-zip and not on mine ?

That one works fine with the same OS and browsers

Touffy commented 2 years ago

Ah ! I think I get it. You ran the build script (npm run start) to generate the worker.js file after cloning from git. But the build script is actually not enough. I haven't yet managed to configure terser to output a correct worker script that can be loaded with importScript, so I always edit the file manually after the script. You have to get worker.js from npm, some CDN, or from a release on GitHub, not make it yourself (or if you do, remove the module wrapper). Sorry.

I've written a short explanation for the CSP stuff down there in the README.

wlinna commented 2 years ago

Thanks!

I just replaced the self-built JS-files with the ones (index.js, worker.js) from the latest release, and that alone fixed the problem. No need to deal with CSP separately, at least not with python3 -m http.server.

Should I leave this issue open? I suppose I should, since downloading the prebuilt scripts is just a workaround.

Touffy commented 2 years ago

It would be nice if I (or somebody else) could fully automate the build. So yes, I suppose we can leave the bug open to track that. In that case, would you mind editing the issue to reflect exactly what's wrong ?

(otherwise I'll close this and open another issue)

wlinna commented 2 years ago

I will edit the issue, no problem. UPDATE: I edited already. Feel free to tell If it needs improving

This is off-topic, but I'd like to quickly ask if I really have to use a form with the Service Worker method to get the RAM saving benefit? I suppose if I made a POST request with fetch with a JSON body, and then turned the result into a blob URL to save it, the blob would be stored to RAM?

Touffy commented 2 years ago

As you say, if you fetch the response into a Blob and then download it with a Blob URL, you lose the advantage of streaming so you might as well not use a Service Worker at all.

The form is convenient because it allows you to "navigate" with a POST request (the browser downloads instead of navigating thanks to the Content-Disposition header). POST is very useful for sending lots of data to the worker to configure what you want in the ZIP file.

If you want to download from a ServiceWorker without a form, you will need to create a download link. But that will make a GET request, and without any custom headers. So you will be limited to just query params. It could make very long URLs, and also open a vector for XSRF attacks.

A way around the limitation (and vulnerability) of query params is to first communicate with the Service Worker over another channel (postMessage) to configure the download, and then click the download link. I actually implemented that before I found the form solution, and decided to show the form in my demo because it is simpler.

wlinna commented 2 years ago

Okay, thanks for confirming. Forms are indeed more convenient for downloading. One thing I was hoping to get by fetching and blobbing is the ease of detection of a download completing. The form doesn't give me that automatically, but maybe I don't need that feature anyway. And if I do, maybe I'll postMessage from the service worker will do.