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

Remove DioMixin #2081

Draft
wants to merge 4 commits into
base: 6.0.0
Choose a base branch
from
Draft

Conversation

kuhnroyal
Copy link
Member

Just a quick hack to figure out if we can get rid of DioMixin and if that would actually make sense.
The DioMixin is currently the main class. The DioForNative and DioForBrowser classes contain minimal logic for download handling.
We can extract this logic into some new platform specific DownloadAdapter in the same way the HttpClientAdapter works.
At the same time we can combine the DioMixin and Dio classes.
This should overall reduce the complexity of having 4 different Dio* classes

I think this would be a good change, not totally happy with passing 2 more parameters to the download handler yet.
What do you think @cfug/devs?

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 (if any)

@kuhnroyal kuhnroyal self-assigned this Dec 28, 2023
@ueman
Copy link
Contributor

ueman commented Dec 28, 2023

I actually like this

@kuhnroyal kuhnroyal force-pushed the feature/download-adapter branch from 378eba1 to 3eeecfa Compare January 2, 2024 14:47
@kuhnroyal
Copy link
Member Author

@AlexV525 Would also love your opinion on this idea.

@AlexV525
Copy link
Member

@kuhnroyal Sorry for the late review. I've looked at why DioMixin was introduced and it seems to be the idea to implement platform-specific requests, starting from here: 6329315#diff-481f5993a7795ed1ee31f808d11a579a48fd60d698e8ca13f85dcb3acff5d421

So basically my two cents:

  1. Removing DioMixin looks great at this point, we have already done this (platform-specific implementation) in many ways.
  2. mixin is still useful. It would be great if the DownloadAdapter could somehow convert to mixin class DioDownloadMixin on Dio, and maybe to avoid duplicate parameters.

@kuhnroyal
Copy link
Member Author

2. mixin is still useful. It would be great if the DownloadAdapter could somehow convert to mixin class DioDownloadMixin on Dio, and maybe to avoid duplicate parameters.

Not sure we can mixin a platform dependent mixin class. Also the adapter pattern allows for custom implementations.

@kuhnroyal kuhnroyal added this to the 6.0.0 milestone Mar 15, 2024
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.

3 participants