search

ipfs / js-ipfs-utils

IPFS utils
Other
23 stars 30 forks source link

Use `XHR` instead of `fetch` to provide progress updates #52

Open Gozala opened 4 years ago

Gozala commented 4 years ago

fetch API does not provide an API to monitor upload progress which prevents ipfs-webui from reporting any progress on file add.

There are some discussions around making progress observable:

It appears that ReadableStreams will at some point provide a way to do that, however no browser supports it today.

In the meantime I'd like to propose using XHR & it's progress events.

Gozala commented 4 years ago

This would imply swapping following code path:

https://github.com/ipfs/js-ipfs-utils/blob/61c7fe23acc8b00627d303cbf8c948e0a03c2b48/src/http.js#L147-L151

with something along the lines of

https://github.com/Gozala/js-ipfs-lite-http-client/blob/66ef08a39aa8ba9bda4406d32583ec3c80850e8a/src/client.js#L27-L99

Gozala commented 4 years ago

Reopening this issue as it had to be reverted #58, as it broke pub / sub.

Gozala commented 4 years ago

Moving some of the conversation from #54 here

From @achingbrain

I think we might have to revert this - the upload/download progress handlers are useful but XHR has no way of doing streaming downloads so it's broken pubsub in the browser as it works via long-lived requests and the call to make the request doesn't return until the load event occurs, eg. the request has finished.

Sorry for the complications, I was not aware of this.

You can access partially loaded content via the .responseText property when .readyState === 3, but only when the response type is text which breaks when the response is non-printable binary data and .responseText grows over time causing a memory leak so it's not much of a solution.

That is how we used to do that back in pre-fetch days. However since I imagine on pubsub response body to be unbound, it's probably not going to be viable option.

@Gozala any ideas on this?

From the sound of it, we only care about streaming body on pubsub, in all other cases getting body via XHR seems reasonable. Therefor instead of trying to workaround XHR, we could probably have an option to choose between progress events and streaming response. That way pubsub can continue using fetch and file upload can opt-in into XHR via progress events. Added complexity is unfortunate, but I don't think there is anything better we can until ReadableStream's as request bodies are supported by browsers.

Gozala commented 4 years ago

@achingbrain what do you think about using XHR if options.onUploadProgress or options.onDownloadProgress is provided, otherwise use fetch ?