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 support for download function in web #2230

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mbfakourii
Copy link

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info

In this PR, download support was added to dio with the help of Blob files.

Note: There is no need to send savePath on the web and it can be empty.

@mbfakourii mbfakourii requested a review from a team as a code owner June 2, 2024 14:38
@mbfakourii mbfakourii changed the title Add support web download Add support download function in web Jun 2, 2024
@kuhnroyal
Copy link
Member

This looks interesting, thanks! I read about createObjectUrlFromBlob just the other day and thought the same.

We will need some tests and and likely an example.

Copy link
Member

@kuhnroyal kuhnroyal left a comment

Choose a reason for hiding this comment

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

Probably missing some error handling.

dio/lib/src/dio/dio_for_browser.dart Outdated Show resolved Hide resolved
Copy link
Member

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

Please write up a general idea of what behavior/standard it follows.

dio/lib/src/dio/dio_for_browser.dart Outdated Show resolved Hide resolved
dio/lib/src/dio/dio_for_browser.dart Outdated Show resolved Hide resolved
dio/lib/src/dio/dio_for_browser.dart Outdated Show resolved Hide resolved
dio/lib/src/dio/dio_for_browser.dart Outdated Show resolved Hide resolved
@AlexV525 AlexV525 marked this pull request as draft June 3, 2024 02:12
@mbfakourii
Copy link
Author

@kuhnroyal @ueman @AlexV525

By further examining the web request, I realized that we use arraybuffer when requesting the web. XMLHttpRequest supports blob. I made a change in the implementation

The problem of high memory is solved.

thank you for checking again

@mbfakourii mbfakourii changed the title Add support download function in web Add support for download function in web Jun 3, 2024
Copy link
Contributor

github-actions bot commented Jun 4, 2024

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
dio/lib/src/adapters/browser_adapter.dart 🟠 75% 🟠 73.25% 🔴 -1.75%
dio/lib/src/dio/dio_for_browser.dart 🟠 57.14% 🟠 50% 🔴 -7.14%
plugins/http2_adapter/lib/src/connection_manager_imp.dart 🟠 60.47% 🟢 79.84% 🟢 19.37%
Overall Coverage 🟢 80.48% 🟢 81.31% 🟢 0.83%

Minimum allowed coverage is 0%, this run produced 81.31%

@mbfakourii mbfakourii marked this pull request as ready for review June 5, 2024 05:25
Copy link
Member

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

Still need some tests and examples here... 🛎️

dio/lib/src/options.dart Outdated Show resolved Hide resolved
dio/lib/src/dio/dio_for_browser.dart Outdated Show resolved Hide resolved
dio/lib/src/adapters/browser_adapter.dart Outdated Show resolved Hide resolved
@AlexV525
Copy link
Member

Could you move on with the new structure of the code and add tests?

@mbfakourii
Copy link
Author

mbfakourii commented Jun 18, 2024

Could you move on with the new structure of the code and add tests?

changes are applied.

thank you for checking again

Copy link
Member

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

Tests are still lacking.

Comment on lines 45 to 46
/// Get blob url.
/// Only work in web.
Copy link
Member

Choose a reason for hiding this comment

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

The description seems to lack explanation.

Copy link
Author

Choose a reason for hiding this comment

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

I made changes to the description

Comment on lines 69 to 72
if (options.responseType == ResponseType.blobUrl) {
completer.complete(
ResponseBody.fromString(
Url.createObjectUrl(xhr.response),
Copy link
Member

Choose a reason for hiding this comment

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

Could you reuse other fields by constructing the ResponseBody with the unnamed constructor?

Copy link
Author

Choose a reason for hiding this comment

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

The ResponseBody with the unnamed constructor requires steam, we have a url string !

dio/lib/src/dio/dio_for_browser.dart Outdated Show resolved Hide resolved
@mbfakourii
Copy link
Author

mbfakourii commented Jul 8, 2024

changes are applied.

thank you for checking again

@AlexV525
Copy link
Member

Please add tests.

@mbfakourii
Copy link
Author

mbfakourii commented Sep 3, 2024

Please add tests.

I tried to add the test, but I get the following error

PS C:\Users\aaa\bbb\dio\plugins\web_adapter> flutter test
Error on line 10, column 3 of dart_test.yaml: Unknown platform "chrome".
   ╷
10 │   chrome:^^^^^^

or

PS C:\Users\aaa\bbb\dio\plugins\web_adapter\test> flutter test .\browser_test.dart
Changing current working directory to: C:\Users\aaa\bbb\dio\plugins\web_adapter
Error on line 10, column 3 of dart_test.yaml: Unknown platform "chrome".
   ╷
10 │   chrome:^^^^^^

And I commented the contents of the "dart_test.yaml" file and run the tests again

PS C:\Users\aaa\bbb\dio\plugins\web_adapter> flutter test
No tests ran.
No tests were found.

or

PS C:\Users\aaa\bbb\dio\plugins\web_adapter\test> flutter test .\browser_test.dart
Changing current working directory to: C:\Users\aaa\bbb\dio\plugins\web_adapter
No tests ran.
No tests were found.

@AlexV525
Copy link
Member

I tried to add the test, but I get the following error

Try to set up the test like the GitHub workflow of this library.

@AlexV525 AlexV525 marked this pull request as draft October 19, 2024 11:21
…orBrowser

# Conflicts:
#	plugins/web_adapter/lib/src/adapter.dart
#	plugins/web_adapter/lib/src/dio_impl.dart
@mbfakourii
Copy link
Author

Please add tests.

Test added

@mbfakourii mbfakourii marked this pull request as ready for review October 30, 2024 05:57
@mbfakourii mbfakourii requested a review from a team as a code owner October 30, 2024 05:57
RequestOptions(
baseUrl: urlPath,
data: data,
method: 'GET',
Copy link
Member

Choose a reason for hiding this comment

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

Why always GET?

Comment on lines +43 to +48
final completer = Completer<Response>();

// Set response in Completer
completer.complete(response);

return completer.future;
Copy link
Member

Choose a reason for hiding this comment

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

These are unnecessary.

Copy link
Member

@AlexV525 AlexV525 Dec 16, 2024

Choose a reason for hiding this comment

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

What will the result be after downloaded? Could you demonstrate how it works?

EDIT: Please also add an example somewhere in our examples so everyone can run and see how it works.

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.

4 participants