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

Allow the Dart team (or any dependency) to deprecate APIs without breaking the tree #143312

Closed
matanlurey opened this issue Feb 12, 2024 · 13 comments
Assignees
Labels
a: annoyance Repeatedly frustrating issues with non-experimental functionality c: proposal A detailed proposal for a change to Flutter c: tech-debt Technical debt, code quality, testing, etc. engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@matanlurey
Copy link
Contributor

I'm not going to spend too much time on this issue, but I think it should be filed for transparency before making any change.

Over the years, we've closed the Flutter tree(s) when a dependency, usually (but not always, sometimes a package) when the Dart SDK introduces a deprecation. This typically causes a cascade of reverts, filed P0s, and landings of // ignore: deprecation, of which Flutter currently has 150+ of these.

Like most style and repository decisions, there are a set of tradeoffs you get. For example, this tradeoff puts a lot of pressure on the Dart SDK team and the Dart engine team/rollers (we're often the first ones to see a failure and have to revert, stop the roller, notify the Dart team), discourages the Dart team from deprecating old or brittle APIs, etc. On the other hand, it's true that we risk an API being deprecated without any working replacement (but that's also true with the current system - see also flutter_driver).

There is not a perfect system, there is perhaps no better example than the fact we don't apply this standard to ourselves. I'm planning to work with @Piinks and @goderbauer on periodically reviewing deprecation warnings, and we'll track it similar to any other form of technical debt that doesn't turn the tree red (Gradle versions, XCode on CI, the list goes on).

For posterity, here are some use cases that become worse after this change:

It's easier to introduce new usages of existing (deprecated) code

Previously, imagine that dart:foo#bar() was deprecated.

Someone would go into all call-sites, and add the following:

import 'dart:foo';

void main() {
+ // ignore: deprecated_member_use
  bar();
}

Then, subsequent additions of bar() would break CI.

Mitigation: None. However, the only deprecated members we break on are dependencies to Flutter, which happen to be packages owned by the Dart or Flutter teams. That is, we can rely on the fact that the previous APIs (a) work and (b) will get cleaned up when the APIs are eventually removed.

We could add tooling to detect "new introductions" of deprecated members, but that is non-blocking to this issue.

It's harder to track how many deprecations exist

Previously, you could grep for deprecated_member_use.

Mitigation: Now, you'd have to change (locally) analysis_options.yaml.

We could add tooling to report the numbers to a dashboard of some sort, similar to other forms of technical debt.

IDE support becomes "all or nothing"

One option that was discussed was making deprecations a hint (non-warning/error), and making hints non-breaking.

However, team members were not excited about the idea of 10s or 100s of diagnostics existing in the IDE.

Mitigation: N/A, we didn't choose this approach.

You could filter hints out of the IDE if you're not working on a file, or we could add custom tooling.


Feedback is welcome, but of course with all forms of feedback, we can't guarantee that it will change the outcome.

I'll shortly file a follow-up issue for a request for better tooling to track deprecations.

@matanlurey matanlurey added team Infra upgrades, team productivity, code health, technical debt. See also team: labels. c: proposal A detailed proposal for a change to Flutter c: tech-debt Technical debt, code quality, testing, etc. labels Feb 12, 2024
@matanlurey matanlurey self-assigned this Feb 12, 2024
@matanlurey matanlurey added framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine repository. See also e: labels. package flutter/packages repository. See also p: labels. a: annoyance Repeatedly frustrating issues with non-experimental functionality labels Feb 12, 2024
@johnmccutchan
Copy link
Contributor

LGTM. I'm very excited to see us reducing churn work for our team.

In the long run, a custom tool that can detect and label all uses of deprecated members seems useful as we can use it during technical-debt reduction sprints.

@leafpetersen
Copy link
Contributor

LGTM as Dart language lead.

@Piinks
Copy link
Contributor

Piinks commented Feb 12, 2024

This L-very-GTM as tech debt lead for the framework. 👍

@jacob314
Copy link
Contributor

LGTM for Dart & Flutter Developer Experience. Long term having tooling to detect new uses of deprecated members would be useful. A team google3 has asked for that sort sort of tooling as they struggle with people on their team accidentally adding additional uses of apis they have deprecated and are working to remove all uses of.

@sigmundch
Copy link
Contributor

LGTM on behalf of Dart Web.

@a-siva
Copy link
Contributor

a-siva commented Feb 12, 2024

LGTM from the Dart vm team side.

@jonahwilliams jonahwilliams added the team-engine Owned by Engine team label Feb 12, 2024
@stuartmorgan
Copy link
Contributor

I'm planning to work with @Piinks and @goderbauer on periodically reviewing deprecation warnings, and we'll track it similar to any other form of technical debt that doesn't turn the tree red (Gradle versions, XCode on CI, the list goes on).

Is there a concrete plan for this? It's frequently the case that technical debt issues that don't have some kind of externally imposed deadline get deferred indefinitely, and if we don't have a robust system in place to find and fix deprecations on a regular basis then we'll have the same kind of problem we have now where rolls are blocked, just at the last possible minute (when we try to roll in the removal of a deprecation we've either ignored or didn't know about for the lifetime of the deprecation), and won't have substantially improved anything.

@goderbauer
Copy link
Member

LGTM as framework TL

I'd love to see some tooling that tracks the usage of deprecated APIs. Plus, it would be awesome to have tooling that warns you when you add more usages of a deprecated API, but doesn't bother you with the grandfathered-in old usages of those APIs.

@matanlurey
Copy link
Contributor Author

Is there a concrete plan for this?

We're adding a bullet-point to the current @Piinks-run process to review technical debt, similar to TODOs.

We'll have the same kind of problem we have now where rolls are blocked, just at the last possible minute

The Dart team is already responsible for (and responsive to) dealing with API removals/breakages, both for Flutter and google3, so I trust them to continue to do a good job here. This proposal is really just "let the Dart team iterate the same way they do in google3 or for non-Flutter clients".

I'd love to see some tooling that tracks the usage of deprecated APIs.

➕ 100. I think this can be handled by the current team that reviews code health (and I'm happy to contribute as well).

@stuartmorgan
Copy link
Contributor

We're adding a bullet-point to the current @Piinks-run process to review technical debt, similar to TODOs.

Does that process cover all affected repos? E.g., will flutter/packages be reviewed by that process, or does this involve new work that the ecosystem team needs to plan for?

This proposal is really just "let the Dart team iterate the same way they do in google3 or for non-Flutter clients".

It isn't just that though. For instance, this will change the way deprecation is flagged within inter-dependent packages in flutter/packages, so we will lose signal that we currently have in that repository.

@jonahwilliams jonahwilliams added P1 High-priority issues at the top of the work list triaged-engine Triaged by Engine team labels Feb 12, 2024
@matanlurey
Copy link
Contributor Author

I've created a catch-all issue for "I wish we had X tooling or process" @ #143344.

matanlurey added a commit to flutter/engine that referenced this issue Feb 13, 2024
Namely, without breaking the tree. This is a deliberate policy decision
change.

See flutter/flutter#143312.
matanlurey added a commit that referenced this issue Feb 13, 2024
… in (#143347)

Namely, without breaking the tree. This is a deliberate policy decision
change.

See #143312.
auto-submit bot pushed a commit to flutter/packages that referenced this issue Feb 14, 2024
…in (#6111)

Namely, without breaking the tree. This is a deliberate policy decision change.

See flutter/flutter#143312.
@Hixie
Copy link
Contributor

Hixie commented Feb 14, 2024

I don't object to this change in policy, but such sweeping changes in long-standing policy should at least be brought to my attention before landing, and should also be flagged on the team chat (#hackers or at least #hidden-chat), and probably announced in #announcements. Transparency is one of our key values.

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: annoyance Repeatedly frustrating issues with non-experimental functionality c: proposal A detailed proposal for a change to Flutter c: tech-debt Technical debt, code quality, testing, etc. engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

No branches or pull requests