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

version 9.8.0 breaking change makes all my pipelines fail #1676

Closed
eugenioadapta opened this issue Aug 17, 2022 · 11 comments
Closed

version 9.8.0 breaking change makes all my pipelines fail #1676

eugenioadapta opened this issue Aug 17, 2022 · 11 comments

Comments

@eugenioadapta
Copy link

Describe the bug
My project uses this import in pubspec.yaml
flutter_local_notifications: ^9.2.0
auto-upgrade inside my pipeline will move to 9.8.0 since you DID NOT mark as breaking change
Now all build actions fail
'''
Warning: The plugin flutter_local_notifications requires Android SDK version 33.
fix this issue by adding the following to /Users/runner/work/blabla/android/app/build.gradle:
android {
compileSdkVersion 33
...
}
'''

To Reproduce
use any project that has compileSdkVersion <33
include in pubspec.yaml
flutter_local_notifications: ^9.2.0

Expected behavior
should not upgrade to 9.8.0, but stay at 9.7.0
because 9.8.0 will make the project break

@MaikuB
Copy link
Owner

MaikuB commented Aug 17, 2022

I understand the point you're trying to make but semantic versioning (https://semver.org) mentions major version changes (i.e. breaking changes) are based on if there are incompatible API changes. There weren't incompatible API changes via the plugin's API surface as part of the release, rather this was more on configuration and having the appropriate SDK version installed. Furthermore, I took a look at what other Flutter plugins within the ecosystem were doing and can see changes to compileSDKVersions as a were done as patch releases e.g. version 3.2.5 of cloud_functions and a fair number of the official Flutter plugins https://github.com/flutter/plugins/search?l=Markdown&q=compileSdkVersion

@bartekpacia
Copy link
Contributor

Now all build actions fail

This is a warning, so you probably must've configured your pipeline to treat warnings as fatal. Maybe try disabling it temporarily?

@MaikuB
Copy link
Owner

MaikuB commented Aug 21, 2022

@bartekpacia it's not a warning. You can reproduce this using this plugin or with the example app of the cloud_functions plugin. I left this issue open so others know. Currently don't intend to revert the change based on what I mentioned earlier

@cubuspl42
Copy link

@MaikuB It's an open problem what should be understood by "API" as the phrase is used in the Semver specification.

I believe that a "backward-incompatible API change" in Semver should be understood as "whatever breaks your dependents after they upgrade, assuming they reasonably used the component as documented, without reaching to its internals or unreasonably relying on specific behavior that was never guaranteed". In consequence, an API here would mean "whatever the public surface of your component is, including transitive stuff".

(Note that the linked issue is closed for "personal" reasons; it was never fixed or categorized as "won't fix")

@cubuspl42
Copy link

In the bigger picture, not breaking your dependents is the sole purpose of the existence of the major version in Semver. The issue is that people have an old habit of treating major versions as something of aesthetic nature and tend to either keep the number low, or link it with some "big phase" of project's development, or i-th partial rewrite, or something that should be a big event when released. All at the cost of breaking their dependents, which is madness if you asked me.

You are right, that the Dart/Flutter package ecosystem doesn't have a perfect culture of respecting their dependents, but it's not really an argument for doing the same.

Summing up, I would encourage you to bump the major version every time you introduce a backward-incompatible change that breaks your dependents (at compile time, or worse, at run-time) when they used your plugin reasonably.

You can keep a version log of major versions in the readme, or in a separate file, so people can know what the upgrading strategy is. Sometimes it can be as easy as "Upgrade your Android SDK to X.Y" or as hard "Migrate all your app to a totally new plugin API".

Thank you for creating this useful plugin, we've been using it in our commercial app since not long after Flutter came out of beta.

@MaikuB
Copy link
Owner

MaikuB commented Aug 25, 2022

@cubuspl42 I'm aware of that and can I understand why it can be argued to be a breaking change. I provided information what Google themselves were doing to give more context on why I originally didn't do this as part of a major release. It wasn't to say I don't understand the viewpoint you mentioned. Nor was it to say I'm just copying what they're doing blindly, rather I took it to meant the community was likely looking at definition of a public API was what's exposed by via the Flutter/Dart APIs of a package.

Whilst I could look at publishing say 9.8.1 to undo the change, I would think it's too late now give as that in itself could cause issues for those who had already updated and to note that going forward is to err on side of caution and publish such future changes with a major version bump. One thing I'd be interested to know if what Google have stated in these circumstances before as I would think there's more in the community using the official plugins that an issue like this would've been raised before

@cubuspl42
Copy link

As far as I understand, we both agree that it is a breaking change, not only that it can be "argued" to be one.

Google (or that micro part of Google that's responsible for Flutter) is just in the wrong here, in my opinion. Unless there's some bigger justification from their side available, besides that they just done that, because "why not".

I do agree that it's not of the biggest importance to undo the change in this case; I would consider that a viable option for a run-time breaking change. I'm just trying to respectfully share my perspective, so next breaking changes get a major version bump.

@MaikuB
Copy link
Owner

MaikuB commented Aug 25, 2022

I'm just trying to respectfully share my perspective, so next breaking changes get a major version bump.

Noted and that's how I've interpreted it.

I'll still keep the issue open for, say, another week or so should others run into such issues

@kimmanwky
Copy link

Indeed, it should be treated as breaking changes, because it requires compileSdkVersion and targetSdkVersion to be upgraded to 33. New version, new standard. Nobody knows what will happens after the version upgrade. It will also affect all the other packages that previously doesn't support or use SDK version 33.

I will need to use flutter_local_notifications: 9.7.1 instead of flutter_local_notifications: ^9.7.1, so that it will not automatically get the version higher than 9.7.1.

@MaikuB
Copy link
Owner

MaikuB commented Sep 8, 2022

@kimmanwky you don't need to change the target sdk version. Target SDK is what changes the behaviour not the compile SDK version

@MaikuB
Copy link
Owner

MaikuB commented Oct 4, 2022

Closing this as I've not seen others raise this issue again

@MaikuB MaikuB closed this as completed Oct 4, 2022
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

No branches or pull requests

5 participants