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

Collection: Major version, use 3.0 language features. #859

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Feb 12, 2025

Move package:collection to using Dart 3.0 language features, in particular final and interface classes.

Remove deprecated APIs.
Deprecate a few new APIs for various reasons (like better versions being available in the platform libraries).
Includes a potentially breaking change to SetEquality and MapEquality which should increase performance.

Adding final or interface to classes requires a major version increment.
That will be incredibly painful to land since package:test depends on collection ^1.x.0 and everything in the world, this package included, depends on package:test. (Half of that world depends on package:collection directly as well, it's not just test.)

For now (to be able to run tests), the pubspec version is 1.x.0-2.0.0.wip.

(The CanonicalizedMap class is only marked @sealed, not final, until an existing subclass can be fixed.
It's unknown how many other packages will be broken by these class modifiers.)

lrhn added 6 commits February 12, 2025 20:03
Keep `.whereNotNull`. It's deprecation is fairly new.
Deprecate `IterableZip` in anticipation of getting `.zip`
in the platform libraries.
Keep `.whereNotNull`. It's deprecation is fairly new.
Deprecate `IterableZip` in anticipation of getting `.zip`
in the platform libraries.
Starts using Dart 3.0 class modifiers.
This is a breaking change, making some classes be `final` or `interface`
which prevents behavior that was previously possible.

Removes existing deprecated API, except for `whereNotNull`.

Deprecates API that is available in platform libraries (`IterableZip` class).

Changes behavior of `UnorderedIterableEquality`, `SetEquality` and `MapEquality`
to assume the compared collections' notions of equality agree with the
`Equality` object for elements/keys in the `UnorderedIterableEquality`/
`SetEquality`/`MapEquality` object.
This should improve performance of the equality comparisons, which currently
try to assume nothing, and therefore cannot use `.contains`/`.containsKey`/`[]`
on one collection with keys/values from the other collection

Exceptions:
- `CanonicalizedMap` marked `@sealed` instead of `final`.
  Was extended in at least one place. Fix to that package sent.
Copy link

PR Health

Breaking changes ⚠️
Package Change Current Version New Version Needed Version Looking good?
collection Breaking 1.19.1 1.20.0-2.0.0.wip 2.0.0
Got "1.20.0-2.0.0.wip" expected >= "2.0.0" (breaking changes)
⚠️

This check can be disabled by tagging the PR with skip-breaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️
File Coverage
pkgs/collection/lib/src/boollist.dart 💚 100 %
pkgs/collection/lib/src/canonicalized_map.dart 💔 76 % ⬇️ 1 %
pkgs/collection/lib/src/combined_wrappers/combined_iterable.dart 💚 100 %
pkgs/collection/lib/src/combined_wrappers/combined_iterator.dart 💚 100 %
pkgs/collection/lib/src/combined_wrappers/combined_list.dart 💚 100 %
pkgs/collection/lib/src/combined_wrappers/combined_map.dart 💚 100 %
pkgs/collection/lib/src/comparators.dart 💚 99 %
pkgs/collection/lib/src/empty_unmodifiable_set.dart 💚 67 % ⬆️ 10 %
pkgs/collection/lib/src/equality.dart 💚 89 %
pkgs/collection/lib/src/equality_map.dart 💚 100 %
pkgs/collection/lib/src/equality_set.dart 💚 100 %
pkgs/collection/lib/src/functions.dart 💚 100 %
pkgs/collection/lib/src/iterable_extensions.dart 💚 100 %
pkgs/collection/lib/src/iterable_zip.dart 💚 100 %
pkgs/collection/lib/src/priority_queue.dart 💚 99 %
pkgs/collection/lib/src/queue_list.dart 💚 88 % ⬆️ 2 %
pkgs/collection/lib/src/union_set.dart 💚 100 %
pkgs/collection/lib/src/union_set_controller.dart 💚 100 %
pkgs/collection/lib/src/unmodifiable_wrappers.dart 💚 80 % ⬆️ 7 %
pkgs/collection/lib/src/wrappers.dart 💚 84 % ⬆️ 8 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

class IterableZip<T> extends IterableBase<List<T>> {
/// @nodoc
@Deprecated('Use [i1, i2].zip from dart:collection')
final class IterableZip<T> extends IterableBase<List<T>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just do another "dot" release to deprecate and then drop this? Let's double down on the breaking changes.

@jakemac53
Copy link
Contributor

What is the strategy for ecosystem rollout here, given that this package is pinned by flutter?

@lrhn
Copy link
Member Author

lrhn commented Feb 12, 2025

Do not expect this to be able to land as-is.
The good question is "if ever". Can we, in any possible way, create a 2.0 of package:collection when the entire ecosystem uses ^1.x.0 constraints?

Jonas suggested not actually removing anything, just keeping it deprecated and @nodoc.
Adding final is still breaking. We can use @sealed instead. Don't have an annotation for interface.
It means we'll likely never get to using final.

And the changes to SetEquality might still be breaking anyway.

@jakemac53
Copy link
Contributor

Just to elaborate on my comment, there are 3042 packages on pub which depend on package:collection https://pub.dev/packages?q=dependency%3Acollection. @sigurdm @jonasfj It isn't clear if this is transitive or immediate dependencies only which matters a lot here.

Since flutter pins this package, every one of these packages will fail to resolve as soon as flutter ups its own dependency on package:collection.

The potential scale of disruption here is huge, and this is exactly why we did not do breaking changes for these packages for null safety, we decided that releasing the breaking change as non-breaking would be less painful overall.

@jakemac53
Copy link
Contributor

Please mark this PR as a Draft if you don't intend to land it :).

I would definitely argue we shouldn't do this, it is better to just keep the tech debt of the deprecated things forever.

Or, we could delete them in a non-breaking change. I also think that would be better, even though it violates semver.

@jakemac53
Copy link
Contributor

(or an even better alternative, convince flutter to stop pinning deps, or at least have a process for not pinning certain specific deps for specified time periods)

@lrhn
Copy link
Member Author

lrhn commented Feb 13, 2025

Any major version increment of a widely used package is hard.

It probably requires a lot of packages to change their dependencies to >=1.19.0 <3.0.0 when they have validated that they work with both versions.
Then, when everyone has done that, individual packages can start requiring the 2.0.0 version without breaking anything.

That's what we simulate when we fiddle with version constraints of the SDK when publishing a new major version.

Semver doesn't help. It can't, a major version increment is a breaking change, you have to opt in to it.

The alternative is really just to release collection2 as an unrelated package.

Or allow Pub to include two different versions of the same package in one program, so one package can use collection 1.19.0, and another 2.0.0.
That requires package resolution to be configurable per-package.
Could choose to only allow one version pet major version.
(But that's not a short-term fix.)

We could create collection2 as a new package, and collection v2.0.0 exporting that (hard version constraint). Then if we ever manage to get people moved to collection 2.0, we can swap the packages so that collection2 exports collection v2, and deprecate collection2.

@lrhn
Copy link
Member Author

lrhn commented Feb 13, 2025

I would find it incredibly worrisome if we choose to make a breaking change as a minor version increment because we can't figure out how to release a major version increment.
That would be saying that semver doesn't work, and rather than fixing how you use semver (like allowing versions of the same package in one program), we just ignore it and lie about whether the change is breaking.
That may just incentivize people to accept smaller version ranges, rather than ^1.X.0 (since that would no longer mean "Until next breaking change").

Maybe we need a new concept: deprecation-aware semver.
If a package version 2.0.0 can say that it is deprecation compatible with versions 1.17.0 and above, then pub could choose to solve with 2.0.0 if all dependencies require 1.17 or above, and can therefore be assumed to not have deprecation warnings for deprecations that were in those versions.
Like a compatibility window, if you don't use anything deprecated in the version you require, then you will be compatible with any later version that is deprecation-compatible with that version.

We'll have to have more kinds of deprecations if we want to make every later API change, like "this parameter will stop being optional" or "... will change type to", or "this class will become final/interface/base/sealed".

Probably can't get away with "this enum will get more values".

@jonasfj
Copy link
Member

jonasfj commented Feb 13, 2025

The alternative is really just to release collection2 as an unrelated package.

The package name collectibles is available 🤣

The main reason to do a major bump of package:collection to version 2.0.0, would be if we don't want people to use both package:collection and package:collectibles because it increases binary size.
But that's probably not going to be a huge issue here.

Maybe, conflicting extension methods could be a problem, but if you do a release of package:collection where everything is either deprecated or re-exported from package:collectibles then you're probably fine.

Or allow Pub to include two different versions of the same package in one program, so one package can use collection 1.19.0, and another 2.0.0.
That requires package resolution to be configurable per-package.
Could choose to only allow one version pet major version.

I very much think we should do this!

Especially, if packages with a dependency on package:collection declare it's a private-dependency, effectively promising that they won't export types from package:collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants