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

feat: http upload/download progress handlers #54

Merged
merged 4 commits into from
Aug 12, 2020
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jul 29, 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

@Gozala Gozala requested a review from achingbrain July 29, 2020 21:56
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way this can work in node too? Shim in xmlhttprequest (disclaimer - I've never used that module, just Googled it) or fall back to http from node core?

@Gozala
Copy link
Contributor Author

Gozala commented Aug 10, 2020

Is there a way this can work in node too? Shim in xmlhttprequest (disclaimer - I've never used that module, just Googled it) or fall back to http from node core?

@achingbrain can we do this as a followup, so we could unblock webui ipfs/ipfs-webui#1534 ?

@Gozala
Copy link
Contributor Author

Gozala commented Aug 10, 2020

Is there a way this can work in node too? Shim in xmlhttprequest (disclaimer - I've never used that module, just Googled it) or fall back to http from node core?

I also looked at the linked library along with numerous others in on npm and it seems that none support upload / download progress events and in fact non that I have looked support node streams. node-fetch is particular in that regard that it switches ReadableStreams for node streams and has it's own Blob implementation that we don't leverage in node code.

We could fall back to plain http module in node, but I am afraid that will wind up in the same ball park as core of node-fetch
https://github.com/node-fetch/node-fetch/blob/8f406b789ab8464c195be87528c42dfcbd912893/src/index.js (probably bit more). So I want to double check that this is indeed the path you want us to take.

@Gozala
Copy link
Contributor Author

Gozala commented Aug 10, 2020

@achingbrain I have added progress events for node as well, by wrapping around request body if onUploadProgress is provided and a stream event listener for response body if onDownloadProgress is provided. Unfortunately both rely on node-fetch implementation details, however I think this is best we can do without re-implementing most of the node-fetch functionality ourselves.

src/http.js Outdated Show resolved Hide resolved
src/http.js Outdated Show resolved Hide resolved
@achingbrain achingbrain merged commit d30be96 into ipfs:master Aug 12, 2020
@achingbrain
Copy link
Member

Published in v2.4.0

@Gozala Gozala deleted the xhr branch August 12, 2020 20:08
achingbrain added a commit that referenced this pull request Aug 14, 2020
Fixes a regression introduced by #54 - in the browser by default the
response body is interpreted as a string which can become corrupted
when non-printable characters are encountered.
@achingbrain
Copy link
Member

achingbrain commented Aug 14, 2020

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.

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.

@Gozala any ideas on this?

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

Successfully merging this pull request may close these issues.

Use XHR instead of fetch to provide progress updates
2 participants