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

added downloadProgress and uploadProgress #1071

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mehmetyaz
Copy link

I have reviewed the issues discussed previously #728 . There were quite a few changes, so I closed PR and reopened it. I also tackled this feature from scratch.

Now we can listen to events for both download and upload.

Optionally, we can also provide data such as speed / remaining time estimation by recording past events over a certain period of time. We also provide records so that the developer can do the calculations themselves.

Example added: pkgs/http/example/progress.dart

It tested in pkgs/http/test/io/streamed_request_test.dart

It was also tested on the browser side with a pkgs/mock_server and pkgs/http/test/html/progress_test.dart.

I'm not sure if I spent enough time on documentation. I want to update it according to demand.

http.post(
    Uri.parse('https://example.com'),
    downloadProgress: HttpProgress((event) {
        event.lengthComputable;
        event.total;
        event.transferred;
        event.progressRate;
        event.percent([fractionDigits])  
    }),
    
    uploadProgress: HttpProgress.withRecorder((event) {
       
        // additionally
        event.records; // List<(int, DateTime)>;
        event.averageSpeed;   event.lastSpeed;   event.estimatedRemaining;
    }),
    ....
)

@kevmoo @brianquinlan @natebosch

@Mehmetyaz
Copy link
Author

Mehmetyaz commented Dec 5, 2023

I noticed the analyzer info. I will fix it. Also, the pkgs/http/test/html/progress_test.dart test will not be successful. That file should be deleted after the Dart team tests it.

@kevmoo
Copy link
Member

kevmoo commented Dec 5, 2023

Not sure if I like the callback model here...but not sure if there is a better way to do it.

@natebosch @devoncarew – thoughts?

We certainly need to be careful about breaking changes...

@Mehmetyaz
Copy link
Author

Not sure if I like the callback model here...but not sure if there is a better way to do it.

@natebosch @devoncarew – thoughts?

We certainly need to be careful about breaking changes...

Actually, I also feel like there is something not quite right (I think that's why I thought of a different model when I started). I can update if there is any suggestion. The basic logic works, after all, and changing the callback model can be done quickly.

/// To see the usage of the progress handler, see [HttpProgress].
///
/// To see the progress of the download, use [downloadProgress].
final HttpProgress? uploadProgress;
Copy link
Collaborator

Choose a reason for hiding this comment

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

My first impression:

Adding new fields to a public class is definitely going to be breaking:
https://github.com/aloisdeniel/http_extensions/blob/122a3d85632a08e0341e4701bafd0e8780740fab/http_extensions_base_url/lib/src/request.dart#L4

Copy link
Author

@Mehmetyaz Mehmetyaz Dec 6, 2023

Choose a reason for hiding this comment

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

Do you mind if I add this feature by creating a new Request and Client(io & html) classes? In global functions (get, post etc), if xProgress parameters are set, this new type of Client will be instantiated.

Copy link
Contributor

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

Speaking as a dio maintainer:
I would prefer to use the StreamedRequest for requests that need progress listening, rather than adding callbacks at this level. StreamedRequest is generally not available for the Web platform IIRC. It would be better to create a new abstraction class to manage clients, which is why dio first came out with adapters.

I'm willing to hear feedback too.

@AlexV525
Copy link
Contributor

AlexV525 commented Dec 12, 2023

Same as #579

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

Successfully merging this pull request may close these issues.

None yet

4 participants