Skip to content
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

Use XHR instead of fetch to provide progress updates #52

Open
Gozala opened this issue Jul 27, 2020 · 4 comments · Fixed by #54
Open

Use XHR instead of fetch to provide progress updates #52

Gozala opened this issue Jul 27, 2020 · 4 comments · Fixed by #54
Assignees
Labels

Comments

@Gozala
Copy link
Contributor

Gozala commented Jul 27, 2020

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 Gozala added the need/triage Needs initial labeling and prioritization label Jul 27, 2020
@Gozala Gozala self-assigned this Jul 27, 2020
@Gozala
Copy link
Contributor Author

Gozala commented Jul 27, 2020

This would imply swapping following code path:

js-ipfs-utils/src/http.js

Lines 147 to 151 in 61c7fe2

const response = await timeout(fetch(url, {
...opts,
signal,
timeout: undefined
}), opts.timeout, abortController)

with something along the lines of

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

@jacobheun jacobheun added status/in-progress In progress and removed need/triage Needs initial labeling and prioritization labels Jul 30, 2020
achingbrain added a commit that referenced this issue Aug 12, 2020
As per #52 this adds `onUploadProgress` / `onDownloadProgress` optional handlers. Intention is to allow ipfs-webui to render file upload progress when new content is added.

Fixes #52

Co-authored-by: Alex Potsides <alex@achingbrain.net>
@Gozala Gozala reopened this Aug 17, 2020
@Gozala
Copy link
Contributor Author

Gozala commented Aug 17, 2020

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

@Gozala
Copy link
Contributor Author

Gozala commented Aug 17, 2020

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
Copy link
Contributor Author

Gozala commented Aug 17, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants