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

fix: Conflict with v6.x of connectivity_plus dependency #1002

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

chadpav
Copy link

@chadpav chadpav commented Jul 27, 2024

Pull Request

Issue

#1001

Closes: #1001

Approach

Bumped connectivity_plus dependency to 6.0.3 (latest). That package now returns a List of ConnectivityResult so I mapped those results to the ParseConnectivityResult enum and preserved old behavior.

I covered with new unit tests.

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

Copy link

Thanks for opening this pull request!

@chadpav
Copy link
Author

chadpav commented Jul 27, 2024

I just noticed that my formatter settings changed some formatting. Are the format settings published so that I can remove the white noise from my changes?

@mbfakourii
Copy link
Member

@mtrezza

Could you please run the workflow?

@mbfakourii
Copy link
Member

I just noticed that my formatter settings changed some formatting. Are the format settings published so that I can remove the white noise from my changes?

Yes, please change the format to normal

@chadpav
Copy link
Author

chadpav commented Jul 27, 2024

I just noticed that my formatter settings changed some formatting. Are the format settings published so that I can remove the white noise from my changes?

Yes, please change the format to normal

I manually reversed formatting changes. Let me know if there is a way for me to change my formatter settings locally to match the repo standards.

@chadpav
Copy link
Author

chadpav commented Jul 27, 2024

I noticed an issue with my commit when testing locally on my real project. I'm out of time this weekend to work on this so consider this a draft PR until I can resolve the issue on Monday. The issue is that I initialize the connectivity dependency inside the Parse initialize() method but I forgot the checkConnectivity() method can get called without calling the initialize() method. I have a fix locally but will do some more thorough testing on iOS/Android devices before moving forward.

@chadpav chadpav marked this pull request as draft July 27, 2024 17:59
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.73%. Comparing base (c388545) to head (5d67e66).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1002      +/-   ##
==========================================
+ Coverage   43.37%   43.73%   +0.36%     
==========================================
  Files          61       61              
  Lines        3463     3466       +3     
==========================================
+ Hits         1502     1516      +14     
+ Misses       1961     1950      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chadpav chadpav marked this pull request as ready for review July 28, 2024 12:43
@chadpav
Copy link
Author

chadpav commented Jul 28, 2024

Refactored the dependency injection to the constructor of the Parse class. This was the right place for it and it cleaned up the test code as well. Tested in my project on iOS + Android as well.

I used the dart formatter from the command line (the same way your workflow does) and it formatted correctly. I had to disabled Format on Save inside VS Code (just fyi).

@mtrezza it's ready for review.

@chadpav
Copy link
Author

chadpav commented Jul 28, 2024

It looks like connectivity_plus had a number of breaking changes including dropping support for old Dart versions. Are you guys ok to mirror those breaking changes now? I guess we should make this a 9.0 release if we do.

BREAKING FEAT(connectivity_plus): support multiple connectivity types at the same time ([#2599](https://github.com/fluttercommunity/plus_plugins/issues/2599)). ([5b477468](https://github.com/fluttercommunity/plus_plugins/commit/5b4774683d6e186fbd69cf4208302221f52aa54d))
BREAKING FEAT(connectivity_plus): Migrate to package:web ([#2621](https://github.com/fluttercommunity/plus_plugins/issues/2621)). ([fbc8e61c](https://github.com/fluttercommunity/plus_plugins/commit/fbc8e61c4f8996d6ba47622de191a83dc2fe1882))
BREAKING BUILD(connectivity_plus): Target Java 17 on Android ([2413e45e](https://github.com/fluttercommunity/plus_plugins/commit/2413e45e88fa6a431c29f8e6240780e20c405453))
BREAKING BUILD(connectivity_plus): Update to target and compile SDK 34 ([#2701](https://github.com/fluttercommunity/plus_plugins/pull/2701)). ([7ddd749](https://github.com/fluttercommunity/plus_plugins/commit/7ddd74989d2921af706f5e7a1aa32e41159ce13f))
BREAKING REFACTOR(connectivity_plus): bump MACOSX_DEPLOYMENT_TARGET from 10.11 to 10.14 ([#2588](https://github.com/fluttercommunity/plus_plugins/issues/2588)). ([f6fe62d5](https://github.com/fluttercommunity/plus_plugins/commit/f6fe62d5f4d87c93e0f974e96bbd47ff453937d9))

https://pub.dev/packages/connectivity_plus/changelog

@mtrezza
Copy link
Member

mtrezza commented Jul 28, 2024

What would this mean in the context of our compatibility policy?

@chadpav
Copy link
Author

chadpav commented Jul 29, 2024

What would this mean in the context of our compatibility policy?

It looks like the policy requires us to support back to Flutter 3.13 through October 31st 2024 and Flutter 3.10 can drop in 2 days. The connectivity_plus package requires >=3.19 but your workflow tests still pass on 3.16.

Since I added unit tests around the functionality for this dependency I wouldn't wait for 3.16 to drop and do the merge on November 1st and require Flutter >= 3.16.

Do you want me to leave the PR sitting here until November? Or close it?

I have a work around so I can move on.

@mbfakourii
Copy link
Member

@mtrezza

I think it is better to keep PR until November

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.

connectivity_plus plugin latest versions are not supported (again)
3 participants