search

parse-community / Parse-SDK-Flutter

The Dart/Flutter SDK for Parse Platform
https://parseplatform.org
Apache License 2.0
575 stars 194 forks source link

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

Closed chadpav closed 1 month ago

chadpav commented 4 months ago

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

parse-github-assistant[bot] commented 4 months ago

Thanks for opening this pull request!

chadpav commented 4 months ago

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 commented 4 months ago

@mtrezza

Could you please run the workflow?

mbfakourii commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

chadpav commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

chadpav commented 4 months ago

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 commented 4 months ago

@mtrezza

I think it is better to keep PR until November

mbfakourii commented 1 month ago

connectivity_plus has been upgraded at #1014