http icon indicating copy to clipboard operation
http copied to clipboard

Add onSendProgress callback

Open tlueder opened this issue 3 years ago • 19 comments

This fixes the upload part of https://github.com/dart-lang/http/issues/465

I had also tried to include the download progress, but that did not work as expected. And it is why easier to do this while consuming the response stream.

tlueder avatar May 27 '21 15:05 tlueder

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar May 27 '21 15:05 google-cla[bot]

@googlebot I signed it!

tlueder avatar May 27 '21 15:05 tlueder

Hope this gets merged, it would really help

NazarenoCavazzon avatar May 16 '22 22:05 NazarenoCavazzon

@tlueder @jasonroland @piotrsed any updates on this?

tomassasovsky avatar May 16 '22 23:05 tomassasovsky

Hi, I can rebase this. But without a clear path forward I don't see the point.

Currently I just use this plugin from my repro via dependency override, works great.

tlueder avatar May 17 '22 08:05 tlueder

@brianquinlan @devoncarew @natebosch

kevmoo avatar May 30 '23 00:05 kevmoo

Please fix merge conflicts!

kevmoo avatar May 30 '23 00:05 kevmoo

@kevmoo done

tlueder avatar May 30 '23 09:05 tlueder

This looks like a breaking change, and we have been avoiding breaking API changes. This is only breaking for a subset of users, but I don't know what proportion.

We are currently getting through a major version bump without too much difficulty, but that was a version bump without breaking API changes. cc @brianquinlan for thoughts on the practicality of a breaking API change and another version bump.

natebosch avatar May 30 '23 22:05 natebosch

This is a API change for sure but I would argue against a breaking one. This adds a new optional feature. And should not break anything for anyone not using it.

Can you elaborate what you mean by "This is only breaking for a subset of users"? Maybe I can change the code to make sure it won't affect them.

tlueder avatar May 31 '23 06:05 tlueder

This is breaking for classes with overrides of the changed methods.

natebosch avatar May 31 '23 14:05 natebosch

Of course, I haven't even thought about that. Thanks for the information.

tlueder avatar May 31 '23 14:05 tlueder

We are currently getting through a major version bump without too much difficulty, but that was a version bump without breaking API changes. cc @brianquinlan for thoughts on the practicality of a breaking API change and another version bump.

  1. As you said, this is definitely a breaking change (in particular it will definitely break at least package:cronet_http and package:cupertino_http).
  2. I wonder if there is a way to implement this without breaking the API? For example, we could define a new class like:
interface class ProgressTracker {
  void onSendProgress(int loaded, int total);
}

class ProgressTrackingRequest extends StreamedRequest implements ProgressTracker {
   ...
}

And send implementations could check if the Request implements that class and, if so, could update the progress e.g.

if (request is ProgressTracker) {
  request.onSendProcess(...);
}

That's a bit ugly.

  1. I'm not sure why tracking upload progress is more important than tracking download progress. If we are going to make a breaking change then we should do both so we don't have to update the major version just to add the obvious complementary feature.
  2. I'd be fine with another version bump but maybe we should wait a few months and accumulate all the breaking changes into one release? Is there an easy way to do this? Like have a 2.0 branch or something?

brianquinlan avatar May 31 '23 17:05 brianquinlan

  1. Understood, and thanks for your input I was not aware of that.
  2. I'd rather wait for a version bump instead of a "quick fix". But in the end, I don't really care so much about the how this is implemented, because I don't know enough about dart best practice and what side effects this can have. I just need the progress data and if this is done a different way? I'm happy to update my apps if that means I can get rid of the dependency override and rebasing my fork.
  3. It is not that tracking the download is not important, but in contrast to the upload it can be done easily on the consuming side as all the needed data is already available. Just not as callback function. And when I wrote this, 2 years ago, I was not able to do this and my attempts felt just wrong. So I left it at that, but any hints in the right direction are welcome.

tlueder avatar May 31 '23 21:05 tlueder