-
Notifications
You must be signed in to change notification settings - Fork 135
feat: downloadFile progress reporting support #159
feat: downloadFile progress reporting support #159
Conversation
Initial implementation of progress reporting for downloadFile on android
Initial implementation of progress reporting for downloadFile on web
Oh wow! Great to see someone tackling this. I'd be fine merging this in without |
Initial implementation of progress reporting for downloadFile on iOS
After some time the iOS implementation is there, took some tweaking and testing to finally get it right, this is my first time writing in swift so I might have missed something... iOS implementation concerns and decisionsAfter some research first thing that seemed to be really easy to implement was to use task.progress.observe but I ended up using custom The iOS implementation is not in any way throttled just like other implementations. I might tweak/polish some things before opening the draft as pull request. |
Is there no way in returning an Observable instead of a Promise to report the progress? I don't like the event listener approach here because it would be very difficult to track parallel uploads/downloads with that. |
Do you mean
Neither do I, but it's the most non-invasive/non-breaking approach I thought was possible at that moment. Http.downloadFile({ onProgress: (...) }) Would that be sufficient for your use-case? I'm going to do some digging into the capacitor plugin internals to check if there is a way to achieve at least that (geolocation plugin looks promising). The "Observable" approach you mention is a concept that would need to be implemented (your angular/rxjs example in the issue is of course not part of the javascript standard) even before implementing downloadFile progress too or the library would need to adapt some kind of implementation of that... It's kind of outside of the scope of this PR.
In my opinion its not that __difficult__, you get the type and url of the downloaded file in the event, assuming that url is unique then you can easily dispatch your own events. Sure it's not hassle-free but something like that should work, no? function observedDownload(options) {
const { url } = options;
let promise;
const progress = new Observable(observer => {
// This part probably doesn't have to be created for every request
// for the sake of the example lets leave it like that
const listener = Http.addListener('progress', e => {
if (e.type === "DOWNLOAD" && e.url === url)
observer.next(/** e.bytes or whatever **/)
});
promise = Http.downloadFile({ ...options, progress: true });
// assuming success
promise.then(() => {
listener.remove();
observer.complete();
});
}
return [promise, progress];
}
// Somewhere else
const [result, progress] = observedDownload({ url: ... });
progress.subscribe({
next(...) { },
complete() { }
});
await result;
// Or you could even move subscribing to function definition and end up with
const result = await observedDownload({ url: ... }, { next(...) {}, complete() }); Disclaimer: I don't have any experience with using observables and my example can be totally wrong. |
@jkbz64 get the point. That was also a thought of mine that Observables are non-standard but would work very well in that case. The callback method would be even better. But for the release now I am fine with everything tbh to just get started ^^ The workaround you shared is not perfect but a good start. I really didn't think about wrapping all in an Observable, good catch! As soon as I am able to work with it I can share an improved one. |
any progress in this feat? |
@zLinz @muuvmuuv I'm going to open up the PR for review since I couldn't find a way to use callbacks in options and callback method doesn't work there (and breaks). |
Thanks for your work, bro!
Thanks bro! This feature is really awesome,I hope it can be a part of the plugins as soon. |
@jkbz64 I tested your PR on Android & iOS, everything works correctly. |
@thomasvidas Can we merge this PR? I'm sure we need to test it on the Web platform. |
I've been out on vacation for most of this month but I'll be sure to test, merge, and release as soon as I have a bit of time to do so 😄 |
Attempt at partially implementing #2
Introduction
The pull requests covers only downloadFile progress (but also places some foundation for uploadFile implementation and maybe even request support) since it seems like the best candidate for checking progress - when we use it we are probably downloading bigger files using it and it was easiest one to implement (no deep buried logic).
Pull request is marked as draft since I'm not sure if it should or can be merged without at least uploadFile support.
Will also try to implement ios support after getting convention/implementation/approval feedback.
For downloadFile only support we can drop generic interfaces in favor of ones like
download-progress
event or just only useDOWNLOAD
ProgressType.Naming conventions and implementation methods of course are subject to change based on feedback.
This draft might aswell go nowhere if it's not good enough, no hard feelings :)
Usage
API and definitions concers
ProgressType is
'DOWNLOAD' | 'UPLOAD'
and so doesHttpProgressListener
indicate generic interface but only'DOWNLOAD'
is supported at this moment - should it be'DOWNLOAD'
only?Web implementation concerns and decisions
There is no built-in (AFAIK) progress reporting mechanism in standard fetch API so there are two ways:
For the initial support I went with writing simple chunk combiner by using streams and it also is opt-in - default behaviour happens when the
progress
is set to false or is not set at all.Not sure if the implementation should be placed in
web.ts
.Event notifying in the initial implementation is not in any way throttled.
Content-Type of response is currently also not being added to Blob.
It works for most cases I tried, would appreciate some testing though to make sure it works good enough.
Android implementation concerns and decisions
For the android support I went with notyfing listeners in
Http.java
file by using@FunctionalInterface
lambda passed down toHttpRequestHandler
ProgressEmitter
was defined inHttpRequestHandler
, but in order to support requests and uploadFile it would need to be probably defined and passed even farther down inCapacitorHttpUrlConnection.java
or require some kind of rewrite.Just like it is in the web case, android implementation is also not throttled in the initial implementation.