search

isoos / postgresql-dart

Dart PostgreSQL driver: supports extended query format, binary protocol and statement reuse.
https://pub.dev/packages/postgres
BSD 3-Clause "New" or "Revised" License
128 stars 36 forks source link

3.3.0 Introduced breaking changes, but it should have been 4.0.0 #383

Closed dsyrstad closed 2 hours ago

dsyrstad commented 2 hours ago

Version 3.3.0 introduced breaking changes and broke our lib/app. Our pubspec contains:

dependencies:
  # ...
  postgres: ^3.0.8

This caused us to pick up 3.4.0, which includes breaking changes from 3.3.0. Those breaking changes should have gone into a major version change, e.g. 4.0.0. See https://dart.dev/tools/pub/pubspec#version and https://semver.org/spec/v2.0.0-rc.1.html#spec-item-9 .

We understand there's nothing that can be done about this now, but in the future we'd like the API to remain stable within a major release.

isoos commented 2 hours ago

@dsyrstad: Every piece of the legacy API had a deprecation marking on it saying Do not use v2 API, will be removed in next release. The changelog for 3.0.0 was clear: "Deprecated all of v2 API, legacy fallback will be removed in next minor version (3.1.0)." It was meant for making the transition to the new API easier so that one can use both APIs at the same time.

We did not remove it in 3.1, nor in 3.2 but eventually in 3.3.

You can always use a fixed version constraint though, nothing wrong with it. In fact, I would have expected something like that if you rely on an API that is clearly and visibly deprecated not only with annotations, but with their library name legacy.dart.

Technically, we shouldn't have included it in 3.0, but did for smoother migration. You are welcome.

dsyrstad commented 2 hours ago

I understand what you did, and that it was deprecated, but that not how the spec says you're supposed to version. See https://dart.dev/tools/pub/pubspec#version and https://semver.org/spec/v2.0.0-rc.1.html#spec-item-9 .

I don't think everyone reads every comment or change log when they pull in a package. However, we do understand how semantic versioning is supposed to work, so we thought we were safe with ^3.0.8. Not using ^ in a package pubspec creates version dependency difficulties for users of the package.

dsyrstad commented 2 hours ago

And, btw, we had migrated most all of our code to use Sql.named(), except one line, which still worked pre-3.3.0. That failure to migrate one line was an oversight on our part, but was still allowed due to the deprecation.

isoos commented 1 hour ago

@dsyrstad At first we wanted to remove the v2 API entirely for the 3.0 release, but early adopters asked for a better migration path, and this seemed to be the best way to do it while keeping the ability to deliver new features easily. Speaking of all the people involved in that discussion, we all knew semantic versioning in great detail, and we knew that this is not 100% specification when we've made this conscious decision. There is very little need to link to the specification again.

I understand that people may not read every part of the announcement, but I somewhat expect them to read it when they upgrade from v2 -> v3 with a lot of breaking changes in their code. What I don't understand though: what were you expecting when using an API that screamed to not rely on it, especially after roughly 6 months between the two releases (3.0.8 -> 3.3.0).

dsyrstad commented 1 hour ago

@isoos Well, we did pay attention to the notices, and did migrate. However, because of the deprecations, we didn't know that our migrations were incomplete. There were no compile errors.

What were we expecting? I think I mentioned it. We were trusting that packages we use to respect the pubspec rules, which includes proper semantic versioning - specifically revving the major version when a breaking change is made. I don't understand the difficulty in changing a "3.x.x" to a "4.x.x". Doing so would make the built-in pubspec versioning features work as expected.

isoos commented 1 hour ago

I don't understand the difficulty in changing a "3.x.x" to a "4.x.x".

The breaking change happened from 2.x -> 3.0, you should have been migrated there. The legacy API was on borrowed time from the get-go, it required extra effort to use it (separate library namespace, wouldn't work out of the box), and it screamed that you shouldn't rely on it (Dart tools do expose the deprecation messages). v3 was released roughly a year ago, the legacy API removal happened roughly 9 months after that. Plenty of time to migrate.

While at the same time the v3 API had no change that was breaking, there was no reason for it to trigger a major version change.

I think your expectations were wrong.