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
Remove IterableExtensions that have exact equivalents in the SDK #331
base: master
Are you sure you want to change the base?
Conversation
Since Dart 3.0, these extensions are in the SDK, exported from core: firstOrNull, lastOrNull, singleOrNull, elementAtOrNull This package already bumped the version to be above that. So, the deletion will not cause any breakage to users, only potential "unused import" warnings.
@@ -4,6 +4,9 @@ | |||
- Shuffle `IterableExtension.sample` results. | |||
- Fix `mergeSort` when the runtime iterable generic is a subtype of the static | |||
generic. | |||
- Remove `firstOrNull`, `lastOrNull`, `singleOrNull` and `elementAtOrNull()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will HAVE to be a breaking change (v2.0.0) since we are removing things.
Better to have a deprecation release first!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are deprecated, it is impossible to switch to the non-deprecated version. You can't switch from .firstOrNull
to .firstOrNull
.
A removal is fully seamless, on the other hand. Could you check the internal discussion that I CC'd you on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I guess as long as the SDK constraint is correct.
I think we'll still need a breaking change version, though. @natebosch ? @devoncarew ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A removal is fully seamless, on the other hand
This is probably true for the vast majority of code. Code with import 'dart:core' as core;
can have trouble here, but that's pretty rare outside of generated code, and most also have an unprefixed import 'dart:core';
.
We could consider exporting the dart:core
extension here. That should keep it non-breaking for even the edge cases. I think the analyzer will still have a diagnostic - but it might switch from unused import to "all used symbols from this import are also available from another import".
WDYT @lrhn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to not export the extension from dart:core
.
We can, but I don't think it'll ever bring us anything but headache.
And do it in a major version increment release, just to be technically allowed to be breaking, even if we hope that nobody notices.
It's not entirely non-breaking, even if we expose extensions with the same signatures, on
types and name.
If someone uses show
or hide
of IterableExtension
for a reason, or use explicit extension invocation, then it can break when that extension no longer has the members.
Unlikely, not impossible.
At least we dodge the bullet of having the extension names themeslves conflict, because this one is called IterableExtension
and the one in dart:collection
is called IterableExtensions
.
So we could export it. But let's not take that kind of chances.
(And thank deity for lack of consistency.)
I'm more hesitant on the major version rev - to |
Thanks @devoncarew . I don't have an opinion on that, I only added the version bump upon request. At least I think we're in agreement regarding the rest of the change itself and that it should be done in this way, not some other way. |
Since Dart 3.0, these extensions are in the SDK, exported from core: firstOrNull, lastOrNull, singleOrNull, elementAtOrNull
This package already bumped the version to be above that. So, the deletion will not cause any breakage to users, only potential "unused import" warnings.
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.