-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[path_provider] Switch Android to an internal method channel #4617
[path_provider] Switch Android to an internal method channel #4617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Some issues below, but this is a great start.
packages/path_provider/path_provider_android/lib/path_provider_android.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_android/test/path_provider_android_test.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_android/lib/path_provider_android.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_android/lib/path_provider_android.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_android/test/path_provider_android_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bparrishMines for second review per policy.
|
||
dependencies: | ||
flutter: | ||
sdk: flutter | ||
path_provider_platform_interface: ^2.0.0 | ||
meta: ^1.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta
shouldn't be necessary if you are just using it for @visibleForTesting
. You should be able to just import 'package:flutter/foundation.dart'
in the file that uses it. I believe the Flutter SDK provides a version of meta
that we don't want to have version conflicts with later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL! We import meta
in a bunch of plugins, probably mostly for this. We should do a pass to clean that up where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed flutter/flutter#95658
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -2,7 +2,7 @@ name: path_provider_android | |||
description: Android implementation of the path_provider plugin. | |||
repository: https://github.com/flutter/plugins/tree/master/packages/path_provider/path_provider_android | |||
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+path_provider%22 | |||
version: 2.0.9 | |||
version: 2.0.10 | |||
|
|||
environment: | |||
sdk: ">=2.14.0 <3.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that I forgot we needed to bump the Flutter SDK requirement to 2.8 as part of this since it uses Dart plugin registration :(
I don't think it's really fixable unfortunately, unless we:
- revert + publish as 2.0.11
- reland with the SDK bump + publish as 2.0.12
because if we just bump the SDK requirement this incorrect version will be the last one 2.5 users will resolve to. We can wait and see if there's non-trivial fallout from this first. (If it's only a small number of people, we can tell them to temporarily pin a path_provider_android dependency.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting a fair number of reports of this already (flutter/flutter#95706), so we'll need to fix this.
Instead of a full revert, I think we could temporarily change the channel name back since nothing about the channel has changed yet, then change it back with an SDK bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a new PR reverting the channel name. I would need to know if it requires a version bump (with corresponding CHANGELOG.md line) and I guess I also need an exception for new tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR flutter#4617 added a dependency on Dart auto-registration, but forgot to bump the SDK requirement to 2.8 (where that feature was introduced for Android). In order to fix older versions, this restores the previous channel name so that the implementation here is compatible with the default method channel registration used by Flutter 2.5 and earlier. A follow-up will restore the package-specific channel name, but also change the SDK requirement to >=2.8. This must be published first so that the last published version that claims 2.5 compatibility actually works with 2.5. Fixes flutter/flutter#95706
PR flutter#4617 added a dependency on Dart auto-registration, but forgot to bump the SDK requirement to 2.8 (where that feature was introduced for Android). In order to fix older versions, this restores the previous channel name so that the implementation here is compatible with the default method channel registration used by Flutter 2.5 and earlier. A follow-up will restore the package-specific channel name, but also change the SDK requirement to >=2.8. This must be published first so that the last published version that claims 2.5 compatibility actually works with 2.5. Fixes flutter/flutter#95706
PR #4617 added a dependency on Dart auto-registration, but forgot to bump the SDK requirement to 2.8 (where that feature was introduced for Android). In order to fix older versions, this restores the previous channel name so that the implementation here is compatible with the default method channel registration used by Flutter 2.5 and earlier. A follow-up will restore the package-specific channel name, but also change the SDK requirement to >=2.8. This must be published first so that the last published version that claims 2.5 compatibility actually works with 2.5. Fixes flutter/flutter#95706
Eliminates the path_provider_android reliance on the default, shared method channel implementation, in favor of an in-package implementation.
Part of flutter/flutter#94224
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.