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

Make use of new dart api for compute #1974

Merged
merged 6 commits into from
Oct 1, 2023

Conversation

ueman
Copy link
Contributor

@ueman ueman commented Sep 20, 2023

Get rid of a lot of code by using a newer Dart API. Also fix some unrelated issues on the v6 branch.

This targets the v6 branch

@ueman ueman requested a review from a team as a code owner September 20, 2023 18:36
@kuhnroyal
Copy link
Member

Pretty neat to get rid of the custom handling but can we actually raise to Dart 3.0.0? I am all for it :)

@ueman
Copy link
Contributor Author

ueman commented Sep 22, 2023

Pretty neat to get rid of the custom handling but can we actually raise to Dart 3.0.0? I am all for it :)

Technically, this change only needs Dart 2.19. But given, the ecosystem is quite good at being up to date, I would say it's okay.

@AlexV525
Copy link
Member

I would be excited to see records and patterns in our packages if we use 3.0 😮. However, if we do so, we might need to consider maintaining separate branches given the fact that thousands (on GitHub) of projects are using Dio 4.0.

@ueman
Copy link
Contributor Author

ueman commented Sep 29, 2023

I reduced the Dart version down to 2.19. That's still higher than the current, but it should be acceptable.

@ueman ueman self-assigned this Sep 29, 2023
@@ -10,7 +10,7 @@ import '../response.dart';
Dio createDio([BaseOptions? options]) => DioForBrowser(options);

/// Implements features for [Dio] on Web platforms.
class DioForBrowser with DioMixin implements Dio {
class DioForBrowser extends DioMixin implements Dio {
Copy link
Member

Choose a reason for hiding this comment

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

Should we make DioMixin as mixin instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess that makes sense. Not in this PR though.

@AlexV525 AlexV525 added s: feature This issue indicates a feature request p: dio Targeting `dio` package labels Oct 1, 2023
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.

RSLGTM

@ueman ueman merged commit 57a7c10 into 6.0.0 Oct 1, 2023
31 checks passed
@ueman ueman deleted the make-use-of-new-dart-api-for-compute branch October 1, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: dio Targeting `dio` package s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants