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

Add onSendProgress callback #579

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

tlueder
Copy link

@tlueder tlueder commented May 27, 2021

This fixes the upload part of #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.

@google-cla
Copy link

google-cla bot commented May 27, 2021

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).

📝 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 google-cla bot added the cla: no label May 27, 2021
@tlueder
Copy link
Author

tlueder commented May 27, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels May 27, 2021
@NazarenoCavazzon
Copy link

Hope this gets merged, it would really help

@tomassasovsky
Copy link

@tlueder @jasonroland @piotrsed any updates on this?

@tlueder
Copy link
Author

tlueder commented May 17, 2022

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.

},
),
);
response = await ioRequest.close();

Choose a reason for hiding this comment

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

Why to needs close this ioRequest

Copy link
Author

Choose a reason for hiding this comment

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

It's been a while since I've last had a look at this.

But I need the response from the ioRequest for the code that follows and I guess that was the easiest way I could think of.

But if that is not a good idea or best practice I'm all in for improvements.

@kevmoo
Copy link
Member

kevmoo commented May 30, 2023

@kevmoo
Copy link
Member

kevmoo commented May 30, 2023

Please fix merge conflicts!

@tlueder
Copy link
Author

tlueder commented May 30, 2023

@kevmoo done

@natebosch natebosch added the next-breaking-release Issues that are worth doing but need to wait for a breaking version bump label May 30, 2023
@natebosch
Copy link
Member

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.

@tlueder
Copy link
Author

tlueder commented May 31, 2023

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.

@natebosch
Copy link
Member

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

@tlueder
Copy link
Author

tlueder commented May 31, 2023

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

@brianquinlan
Copy link
Collaborator

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?

@tlueder
Copy link
Author

tlueder commented May 31, 2023

  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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes next-breaking-release Issues that are worth doing but need to wait for a breaking version bump package:http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants