feat(connectivity_plus): add checkConnectivity.hasConnectivity() utility getter#3842
feat(connectivity_plus): add checkConnectivity.hasConnectivity() utility getter#3842EchoEllet wants to merge 4 commits into
Conversation
|
This PR does not modify |
69026c5 to
e13be4c
Compare
e13be4c to
958478a
Compare
vbuberen
left a comment
There was a problem hiding this comment.
I like the idea, but there is some work to do before we can merge this.
My main suggestion is to export extension via show as I suggested in one of my review comments.
Additionally, let's add mention and code example with the new extension to the README file and the example app, so it is easy to find for package users.
| /// | ||
| /// ```dart | ||
| /// final result = await checkConnectivity(); | ||
| /// print(result.hasConnectivity); |
There was a problem hiding this comment.
It won't work the way it is described as currently calls like checkConnectivity() are not done without Connectivity() class. And hasConnectivity is only available if there is an import of 'package:connectivity_plus_platform_interface/connectivity_plus_platform_interface.dart' in the file.
So this doc comment is misleading and needs an improvement. I would suggest to move this example comment about hasConnectivity to be above the extension itself.
There was a problem hiding this comment.
I would suggest to move this example comment about
hasConnectivityto be above the extension itself.
ConnectivityResultListX is inside connectivity_plus_platform_interface, so it should probably not assume the API shape of code that depends on connectivity_plus.
I would avoid the reference to Connectivity() entirely inside the platform interface package.
So this doc comment is misleading and needs an improvement.
With the proposed change, consumers cannot find this extension by only reading connectivity_plus.
I think we can add a doc comment to hasConnectivity instead:
extension ConnectivityResultListX on List<ConnectivityResult> {
/// Returns whether any active network connection exists.
bool get hasConnectivity =>
!(length == 1 && first == ConnectivityResult.none);
}And then document it in README.md of connectivity_plus instead?
There was a problem hiding this comment.
Let's add the new extension here to avoid the need for additional import of platform interface by package users.
There was a problem hiding this comment.
This will cause a temporary Dart analysis warning since connectivity_plus depends on connectivity_plus_platform_interface from pub.dev, and ConnectivityResultListX does not exist in the latest published version.
Should I add a temporary dependency override, or leave that to the project maintainers?
We also probably want to validate whether ConnectivityResultListX is a good name, because changing it later is a breaking change.
…lt>.hasConnectivity
…t ConnectivityResultListX in connectivity_plus_platform_interface
3bd89c3 to
966f514
Compare
…Connectivity extension getter
966f514 to
6f6923d
Compare
Description
A very minor feature that is commonly used (e.g., serverpod_flutter):
Related Issues
Checklist
CHANGELOG.mdnor the plugin version inpubspec.yamlfiles.flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
!in the title as explained in Conventional Commits).