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

[dart-dio-next] Switch dio implementation to dio_http because dio is no longer maintained #10305

Closed

Conversation

jgoyvaerts
Copy link

Because https://github.com/flutterchina/dio is no longer maintained by it's contributor, I've decided together with some other contributors to fork it as https://github.com/dart-tools/dio_http. More information can be found here: https://github.com/dart-tools/dio_http/blob/master/FORK.md

Apart from the fact that this fork is maintained and will continue to get bugfixes and improvements, the biggest improvement is the performance fix for web (dart-tools/dio_http@2c243d9) which causes a 13x speedup or more when downloading files on web through dio.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob @ahmednfwela

Copy link
Contributor

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

While I see your point and appreciate you taking the time for this PR and starting the fork, we can not just replace dio with a new and (for now) unknown fork.
wendux merged my PRs for the list parameters and was active to migrate dio to NNBD, I know its been 4 month but we should give him a bit of time.

If you want to add a generator flag along with tests for the flag, then we can consider merging this. But dio needs to stay the default for now.

@jgoyvaerts
Copy link
Author

I don't think openapi-generator needs another dart generator or flag, there's already enough of them (even dart-dio-next is still unstable).

I can see your point regarding the new fork argument, hopefully you'll reconsider your standpoint in a few weeks if the situation stays unchanged.

@kuhnroyal
Copy link
Contributor

even dart-dio-next is still unstable

That's not true, I consider it stable and it is being used by quite a few people. It is marked experimental which only has to do with the next major release of openapi-generator, in which we can replace the current dart-dio generator.

@jgoyvaerts
Copy link
Author

Don't get me wrong, I'm using it too, but there's still issues with it (oneOf/dynamic behavior, nullable enums not being handled properly). I've got a PR coming up for the oneOf fix, but the nullable enums still need fixing. I'll make a separate issue for that

@kuhnroyal
Copy link
Contributor

Great! Looking forward to all the help we can get here.

@ahmednfwela
Copy link
Contributor

I don't see a problem with this, I would rather use a maintained fork than an unmaintained original.
and yet again another reason to work on this dart-lang/pub-dev#4146

@josh-burton
Copy link
Contributor

A flag is a good idea. There are a number of 3rd party packages that depend on dio so for anyone using them it's not a simple switch over.

A flag for which dio to use shouldn't be too much work here?

@jimcullenaus
Copy link

Don't really mind whether it's replacing the original or as an extra flag option. We just want the maintained fork to be available without having to do complex post-generation processing. The performance benefits alone make a huge difference.

@shammill
Copy link

We've swapped to the dio_http fork to resolve some performance issues with the original repo. 5 months is a long time to have significant performance issues.
How much longer should we wait for someone who neglects to check PRs?

@josh-burton
Copy link
Contributor

I still suggest adding a flag to this generator. I would switch to the fork and I'm sure others would too which would drive adoption upwards.

But a breaking change like that is not realistic for this generator.

josh-burton added a commit to josh-burton/openapi-generator that referenced this pull request Sep 29, 2021
wing328 pushed a commit that referenced this pull request Oct 5, 2021
* [dart-dio] Adds an option for using the dio_http package

Relates to #10305

* [dart-dio-next] Generates new dio_http sample

* [dart-dio-next] renames dio_http sample, adds pom.xml

* [dart-dio-next] Removes executions not required
@jgoyvaerts
Copy link
Author

Update on this PR: The dio owner has returned from inactiveness and has added me and some others as contributors to the dio package so this situation doesn't happen again.

Since I have no interest in dividing community efforts and since dio is still the largest and most used package, I have archived dio_http (all fixes should have been ported to dio).

Closing this PR since it's no longer needed.

@wing328 Not sure if the dio_http option is still needed, I guess we should remove it in the near future to clean up the code a bit

@jgoyvaerts jgoyvaerts closed this Nov 22, 2021
@jgoyvaerts jgoyvaerts deleted the update-dio-next-dio_http branch November 22, 2021 12:50
@kuhnroyal
Copy link
Contributor

@josh-burton Do you have time to revert the generator flag?

@ahmednfwela
Copy link
Contributor

I don't think we need to remove it, just leave it for the time being

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.

6 participants