-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Comments
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 |
This is a warning, so you probably must've configured your pipeline to treat warnings as fatal. Maybe try disabling it temporarily? |
@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 |
@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") |
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. |
@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 |
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. |
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 |
Indeed, it should be treated as breaking changes, because it requires I will need to use |
@kimmanwky you don't need to change the target sdk version. Target SDK is what changes the behaviour not the compile SDK version |
Closing this as I've not seen others raise this issue again |
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
The text was updated successfully, but these errors were encountered: