search

parse-community / Parse-SDK-Flutter

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

feat: Add support for Dart 3.1, remove support for Dart 2.18 #969

Closed mbfakourii closed 10 months ago

mbfakourii commented 10 months ago

Pull Request

Issue

Closes: #968

Approach

Improve support policy and CI

Tasks

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

Thanks for opening this pull request!

codecov[bot] commented 10 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (1ae0808) 39.60% compared to head (f1ce3b7) 39.60%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #969 +/- ## ======================================= Coverage 39.60% 39.60% ======================================= Files 60 60 Lines 3333 3333 ======================================= Hits 1320 1320 Misses 2013 2013 ```

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

mtrezza commented 10 months ago

Re-added some comments to the CI, removed unnecessary comments.

mbfakourii commented 10 months ago

Looks good; changed this to feat PR, since it's adding Dart 3.1 support; added breaking label since it's removing Dart 2.18 support. This will publish as Parse Dart SDK 6.0.

Is this ready for merge?

It seems good

mtrezza commented 10 months ago

Could you add the Dart version change?

I just realized this PR has changes for Dart and Flutter, but I think our process so far has been to keep them separate, as they are 2 separate packages.

So this is also a breaking change for the Parse Flutter SDK? If so; the PR title is missing that information. I believe we should keep the process of keeping Dart PRs separate from Flutter PRs, to avoid any confusion in this mono repository, as long as we de these releases manually.

mbfakourii commented 10 months ago

Could you add the Dart version change?

I just realized this PR has changes for Dart and Flutter, but I think our process so far has been to keep them separate, as they are 2 separate packages.

So this is also a breaking change for the Parse Flutter SDK? If so; the PR title is missing that information. I believe we should keep the process of keeping Dart PRs separate from Flutter PRs, to avoid any confusion in this mono repository, as long as we de these releases manually.

I don't think it is necessary because we are changing a higher thing which is support policy. This issue is related to both packages. It is also a type of refactor and no changes have been made to the codes.

Are you sure about this?

mtrezza commented 10 months ago

This PR is more than a refactor, I've explained in https://github.com/parse-community/Parse-SDK-Flutter/pull/969#pullrequestreview-1656281916 why.

We haven't done multi-package changes in a single PR so far. That's something new and we'd need to explore how that affects our current process. I'd rather see these PRs moving forward that being stuck unnecessarily.

mbfakourii commented 10 months ago

This PR is more than a refactor, I've explained in #969 (review) why.

We haven't done multi-package changes in a single PR so far. That's something new and we'd need to explore how that affects our current process. I'd rather see these PRs moving forward that being stuck unnecessarily.

Done

mbfakourii commented 10 months ago

The related errors are due to the marge between Flutter and Dart versions, which will be resolved in the next PR

mbfakourii commented 10 months ago

CI fails with error:

It is due to the interference of two packages. Please merge, it will be fixed.

mtrezza commented 10 months ago

How will it be fixed?

Could you please add a changelog entry and bump the version like in https://github.com/parse-community/Parse-SDK-Flutter/pull/971#pullrequestreview-1663497158?

mbfakourii commented 10 months ago

The problems are solved. Two errors are solved with this PR

mtrezza commented 10 months ago

Thanks, so the Flutter tests are failing because the Parse Flutter SDK uses the unreleased Parse Dart SDK in this repository?

mbfakourii commented 10 months ago

Thanks, so the Flutter tests are failing because the Parse Flutter SDK uses the unreleased Parse Dart SDK in this repository?

Yes, that's correct.

mtrezza commented 10 months ago

I think we have to change that, because merging a PR despite failing tests seems counter-intuitive. It's also taking more time to review a PR, because we always have to look into why a job is failing. We also have several other PR blocked because their tests are failing until this PR is merged.

I believe as long as we don't modify and release both packages with 1 PR (like you had it earlier in this PR, but we need to check the implications), the Flutter tests should use the published Parse Dart SDK, not the local package. What do you think?

mbfakourii commented 10 months ago

I think we have to change that, because merging a PR despite failing tests seems counter-intuitive. It's also taking more time to review a PR, because we always have to look into why a job is failing. We also have several other PR blocked because their tests are failing until this PR is merged.

I believe as long as we don't modify and release both packages with 1 PR (like you had it earlier in this PR, but we need to check the implications), the Flutter tests should use the published Parse Dart SDK, not the local package. What do you think?

It is a good suggestion to use the paths in Github action for this issue

By the way, there's a package called melos. Can you check to see if it's useful for our work?

mtrezza commented 10 months ago

Do you mean we could use paths to exclude the Flutter CI jobs when the flutter files have not been modified?

mbfakourii commented 10 months ago

Do you mean we could use paths to exclude the Flutter CI jobs when the flutter files have not been modified?

No, I mean that the implementation of CI is created based on the changes of a certain folder, for example, we have two ci, one for dart with the package/dart path and one for flutter with the package/flutter path, which can be executed by changing the corresponding path files.

mtrezza commented 10 months ago

We'd have to look into that. I think the issue here is that if someone downloads the repository and runs Flutter tests, they would still fail locally. So we would end up with "half working" releases; with no effect for the developer, but maybe making it confusing for the contributor.

How difficult would it be to make the Flutter SDK use the published Parse Dart SDK instead of the local one?

mbfakourii commented 10 months ago

We'd have to look into that. I think the issue here is that if someone downloads the repository and runs Flutter tests, they would still fail locally. So we would end up with "half working" releases; with no effect for the developer, but maybe making it confusing for the contributor.

How difficult would it be to make the Flutter SDK use the published Parse Dart SDK instead of the local one?

My suggestion is to add range support to Flutter package. Something like this:

parse_server_sdk: '>=3.0.0 <6.0.0'

This would allow Flutter package to support a range of versions, such as 3 to 6.

mtrezza commented 10 months ago

Not sure this would be possible. These are major versions and there may be breaking changes. We already have a range with parse_server_sdk: ^5.1.2, the flutter SDK just needs to use the published version; not the local version.

mbfakourii commented 10 months ago

Not sure this would be possible. These are major versions and there may be breaking changes. We already have a range with parse_server_sdk: ^5.1.2, the flutter SDK just needs to use the published version; not the local version.

We currently do not use the local version. The reason for the CI error is related to the remove of support from version 2.18

The error in Flutter 3.3 is as follows:

ERR : The current Dart SDK version is 2.18.6.
    | 
    | Because parse_server_sdk requires SDK version >=2.19.6 <4.0.0, version solving failed.

We removed 2.18.6 and it is not supported in Flutter 3.3

I think there is no problem !

mtrezza commented 10 months ago

How is this possible if we did not actually remove Dart 2.18 support yet? It's only removed in this PR, not in the published Parse Dart SDK version.

mbfakourii commented 10 months ago

I think the problem was found ! The issue here is that we are checking the Parse Dart package in the Flutter section of CI.

mtrezza commented 10 months ago

Makes sense, can we remove this line from the CI?

mbfakourii commented 10 months ago

Makes sense, can we remove this line from the CI?

I think to remove this line, it is better to do PR flutter.

What is your opinion about the separation of two CI (flutter and dart)?

mtrezza commented 10 months ago

Sure, so should the line be removed in https://github.com/parse-community/Parse-SDK-Flutter/pull/971#pullrequestreview-1674817541?

mbfakourii commented 10 months ago

@mtrezza

What do you think about melos?

mtrezza commented 10 months ago

I think you already identified the solution in https://github.com/parse-community/Parse-SDK-Flutter/pull/969#issuecomment-1759782645. If you just remove the checking in the flutter section of the CI in the https://github.com/parse-community/Parse-SDK-Flutter/pull/971#pullrequestreview-1674817541 PR, then all these changes of splitting up the CI are unnecessary. We also want to keep the CI files as uniform as possible across repositories, for easier maintenance, so creating 2 files here if not necessary should be avoided if possible.

mbfakourii commented 10 months ago

I think you already identified the solution in #969 (comment). If you just remove the checking in the flutter section of the CI in the #971 (review) PR, then all these changes of splitting up the CI are unnecessary. We also want to keep the CI files as uniform as possible across repositories, for easier maintenance, so creating 2 files here if not necessary should be avoided if possible.

But didn't you say to separate it here?

mtrezza commented 10 months ago

Yes, remove the line in the flutter PR https://github.com/parse-community/Parse-SDK-Flutter/pull/971 (or adapt the CI so that Dart is not tested when running the flutter CI, without using path in the CI), so we can merge the flutter PR first, then rebase this PR and there won't be any issues anymore, right?

mbfakourii commented 10 months ago

Yes, remove the line in the flutter PR #971 (or adapt the CI so that Dart is not tested when running the flutter CI, without using path in the CI), so we can merge the flutter PR first, then rebase this PR and there won't be any issues anymore, right?

Done, please check again