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

dio/lib/src/options.dart Outdated Show resolved Hide resolved
plugins/web_adapter/lib/src/html/adapter.dart Outdated Show resolved Hide resolved
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
plugins/web_adapter/lib/src/dio_impl.dart Outdated Show resolved Hide resolved
plugins/web_adapter/lib/src/dio_impl.dart Outdated Show resolved Hide resolved
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.

Copy link
Author

@mbfakourii mbfakourii Jan 17, 2025

Choose a reason for hiding this comment

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

I added an example in example_flutter_app.

Copy link
Author

Choose a reason for hiding this comment

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

To test on the local web, make sure CORS is disabled.

and use local dio and dio_web_adapter in example_flutter_app pubspec

dependency_overrides:
  dio_web_adapter:
    path: ...\dio\plugins\web_adapter
  dio:
    path: ...\dio\dio

Copy link
Member

Choose a reason for hiding this comment

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

Why CORS is required?

Copy link
Author

@mbfakourii mbfakourii Jan 28, 2025

Choose a reason for hiding this comment

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

To download a file from an online server to a local server, the browser gives a CORS error. This has nothing to do with Dio. I said this for testing.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, how about a local server to scope with the minimal functionality instead of an outer source?

Copy link
Author

@mbfakourii mbfakourii Jan 28, 2025

Choose a reason for hiding this comment

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

Do you mean to put a file locally? If we do this, the download display will be very fast and the download percentage will not be clearly visible.

Are you sure about this? This is just an example to demonstrate a feature 🤔

Copy link
Member

Choose a reason for hiding this comment

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

A demo would not require displaying the progress, correct? The file could be as small as possible too.

Copy link
Author

Choose a reason for hiding this comment

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

I think there are some examples for pure Dart

Just need to run it on the web.

Copy link
Author

Choose a reason for hiding this comment

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

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