| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
I'm definitely on board with having recommendations for when to use enum vs. static const.
My main concern is that allowing non-exported code will result in a proliferation of private files, and that users will import them by path regardless of what the styleguide says. This could result in even more time spent dealing with breaking changes than before. Even though we state that we don't consider these to be breaking changes, usage by g3 customers and customer_tests can still disrupt us.
I think that's the key question for me, do we think this could create more problems than it solves?
Some other questions:
Sorry, something went wrong.
|
|
||
| * Declarations marked `@internal` are outside the public API contract; modifying or removing them is not a breaking change. | ||
|
|
||
| * For public APIs, a standard `enum` is used when consumers must handle all values exhaustively (making new values a breaking change), and a class with `static const` fields is used when consumers need a fallback (making new values non-breaking). |
There was a problem hiding this comment.
I agree with this. It's good to have an official recommendation for when to use these. This is even more important now that adding an enum will break the flutter/packages autoroller; we can't accidentally sneak in a new enum value under the radar any more.
Sorry, something went wrong.
| #### Placement rules | ||
|
|
||
| * `@internal` may be applied to top-level declarations and to `static` members or constructors of public classes inside `lib/src/`. | ||
| * In any file in `lib/src/` not exported by a barrel file, all public top-level declarations must be annotated with `@internal`. |
There was a problem hiding this comment.
Good rule to make this explicit.
Sorry, something went wrong.
| ### Removing discouragement of `@visibleForTesting` | ||
|
|
||
| The style guide section "Avoid using `@visibleForTesting`" advises against `@visibleForTesting` on public declarations, preferring APIs testable through their public interfaces, though contributors still use it when necessary. Should the style guide remove its discouragement of `@visibleForTesting` or keep the current stance? |
There was a problem hiding this comment.
I can't use an API marked @internal in a test, right?
This styleguide rule is one I disagreed with when I first started on the team, but I've since gotten used to it. I want to say that I agree with the recommendation to not kid ourselves about visibleForTesting being effectively public, and so we should keep the rule as-is.
Sorry, something went wrong.
There was a problem hiding this comment.
I can't use an API marked @internal in a test, right?
Yes they serve different purposes. so I don't think we can replace visibleForTesting with internal
I want to say that I agree with the recommendation to not kid ourselves about visibleForTesting being effectively public, and so we should keep the rule as-is.
did you mean we should state that changing API that is visibleForTesting is not breaking change, and keep the discouragement as is?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes
Sorry, something went wrong.
| ### Adopting `@experimental` for cutting-edge development | ||
|
|
||
| New public APIs immediately fall under the breaking change policy, requiring an RFC, deprecation cycle, and migration guide to modify or remove. Should Flutter adopt [`@experimental`](https://pub.dev/documentation/meta/latest/meta/experimental-constant.html) from `package:meta` to exempt cutting-edge APIs from the breaking change policy? |
There was a problem hiding this comment.
We already have feature flags for this, I think no change is needed.
Sorry, something went wrong.
There was a problem hiding this comment.
The reason I bring this up is that should we abandon feature flag approach and just mark something as @Experiemental and treat the change to the API as non breaking change?
On the other hand, we can also just use @internal here as well.
Also, using feature flag requires a lot of works as well, is it really worth it if we can just document our rule and lint warning against using these cutting-edge API
Sorry, something went wrong.
There was a problem hiding this comment.
I'd lean towards avoiding @experimental in the Flutter SDK. While @experimental declares that we reserve the right to change an API, @experimental doesn't actually make the change any less impactful. I'd argue the Flutter SDK is so foundational that the impact of changing an @experimental API would be too high. Furthermore, @experimental APIs are listed during code completions, making them much more visible than @internal APIs.
... should we abandon feature flag approach and just mark something as @Experiemental and treat the change to the API as non breaking change?
In my mind, these serve different use cases:
On the other hand, we can also just use @internal here as well.
Agreed 👍
Sorry, something went wrong.
| ### Handling feature flag files and the cross-layer import rule | ||
|
|
||
| `lib/src/foundation/_features.dart` defines `@internal` feature flags consumed by higher layers (for example, `lib/src/widgets/binding.dart` imports `../foundation/_features.dart`). Because `@internal` symbols cannot be exported by `lib/foundation.dart`. What should be done about feature flags? |
There was a problem hiding this comment.
What about the proposal affects this?
Sorry, something went wrong.
There was a problem hiding this comment.
I can think of a few approach.
Sorry, something went wrong.
There was a problem hiding this comment.
I understand that feature flags are an exception to the "no relative imports across libraries" rule, but is this exception a problem in practice? This exception seems to be OK so far?
Sorry, something went wrong.
There was a problem hiding this comment.
yup, I think that is ok to keep it as exception, though we should make sure this doesn't encourage more exceptions
Sorry, something went wrong.
|
Thanks for writing this up. I agree with the underlying problems, but I think the document needs a structural rewrite. Comments are grouped below. 1. Split into separate RFCsI'd suggest splitting this into three RFCs:
I see the common theme, which is keeping APIs evolvable without going through the breaking change process. But the decisions can be made separately. The enum conventions are independent of the other two. The experimental RFC would build on @internal, but only in one direction: @internal stands on its own, so there's no reason for it to wait on experimental APIs. They're also at very different maturity levels. The @internal part mostly codifies existing practice, while the enum and experimental parts raise larger questions. Since an RFC is accepted or rejected as a whole, bundling them means the more settled parts wait on the most contested one. The audiences differ as well: @internal mainly concerns contributors and repo layout, while the enum conventions concern API design for every Flutter user. Experimental APIs deserve their own RFC because they raise questions the other two don't: exempting public APIs from the breaking change policy; how @experimental (a static experimental_member_use warning) relates to the existing runtime gate in _features.dart (driven by the tool's feature flags, so experimental APIs can't be used on stable at all), and whether that gate is still needed; and @internal instance members on public, extensible types (WidgetsBinding.windowingOwner), which the proposed placement rules would themselves forbid. Given the dependency, it makes sense to settle @internal first. 2. @internal / package-private APIsStructure: two proposals with different motivationsI think the argument should be restructured into two proposals:
MotivationThe current Context assumes package-private APIs are already desirable and only argues for adding a guardrail. It doesn't explain why we need them in the first place. The core problem, as I see it: since every non-underscored symbol in lib/src can be imported, the current style guide leaves only two options for something we don't want to keep backward compatible. Either make it library-private with _, which forces all code that uses it into a single file and hurts maintainability, or make it public, which commits us to backward compatibility for what is really an implementation detail. The style guide's rationale is that forcing things public leads to better design, but in practice that cost discourages splitting files and refactoring. The RFC should make this argument explicitly and address that rationale directly (see also flutter/flutter#108632). The background should also mention part/part of and why it's not a viable alternative: it makes every underscored symbol visible across all part files, which removes file-level encapsulation and hurts maintainability even more. It's also worth noting this matches existing practice. lib/src imports are not considered supported across the Dart ecosystem (implementation_imports), and flutter/packages already uses @internal with hide in the way proposed here (e.g. go_router hides GoRouterStateRegistry from its barrel export). Existing _-prefixed files and the filename ruleThe background should explain what the existing _-prefixed files in lib/src are for. Most of them (_*_io.dart / _*_web.dart) are conditional-import implementations and are unexported for structural reasons, not because of API stability. The windowing files (_window*.dart, _features.dart) are experimental APIs and belong in the experimental RFC. That leaves _accessibility_evaluations.dart as the clearest existing framework example for this RFC: an implementation detail shared within the widgets layer (used by binding.dart), with no reason to be public. With @internal as the source of truth, I don't think a _ filename should be required for internal code. The conditional-import convention is out of scope and can stay as it is. @internal vs. @visibleForTestingI'd suggest keeping @internal and @visibleForTesting semantically distinct rather than letting @internal cover test access. @internal means "not part of the public contract, shared within the package"; @visibleForTesting means "would be private, exposed only for tests". Concretely: @internal symbols should not be accessed from unit tests (tests should exercise them through public behavior), and @visibleForTesting remains the way to grant test-only access, which the analyzer already enforces. Note that combining the two annotations doesn't express "usable from other files and from tests", because the analyzer treats them as an intersection. I think that's acceptable, but the RFC should state it explicitly. This makes the open question about @visibleForTesting part of the core proposal rather than a side question, since the two annotations need a clearly defined division of responsibility. LayersWith experimental APIs moved out, the only current cross-layer use of an unexported file (_features.dart) goes with them. This RFC can then simply state that @internal doesn't change the cross-layer import rule, and leave feature flags to the experimental RFC. ScopeI'd suggest explicitly scoping this to both the framework and flutter/packages. flutter/packages already follows this in practice, so this would codify an existing unwritten rule rather than introduce a new one. It would also supersede flutter/flutter#108632. Customer testsThe breaking change policy is defined in terms of the customer test registry, so the exemption needs to be reconciled with it. If a registered test imports package:flutter/src/... and uses a symbol that later becomes @internal, it will fail: most registered suites run flutter analyze --no-fatal-infos, and invalid_use_of_internal_member is a warning, not an info. By the current definition, that would make the change breaking. I'd suggest:
EnforcementFor enforcement, I'd prefer an analyzer plugin over checks in dev/bots/analyze.dart, so the rules work in the IDE and cover flutter/packages as well. flutter/flutter already has dev/flutter_analyzer_plugin for this kind of migration. Two things the RFC should address:
Smaller technical points
3. Public enum conventionsThis isn't a breaking change policy changeI don't think this part changes the breaking change policy at all, so I'd drop the 030-breaking-changes framing from it. Adding a value to an enum breaks exhaustive switches. That's enforced by the compiler, and the policy can't change it. Adding a static const field to a final class with a private constructor isn't breaking, the same as adding any other static member, so it never needed the breaking change process. What this section really proposes is an API design guideline: decide whether a type is semantically closed before reaching for enum. BackgroundThe background should reference dart-lang/sdk#63678, where non-exhaustive enums were requested at the language level. The Dart team's position (comment) is that this is a substantial feature and unlikely to be prioritized in the near future. That's the main reason a library-level convention is needed now. The criterionI'd suggest the criterion be whether the set of values is closed by nature:
When it's unclear, I'd lean toward open. The costs are asymmetric: if an open set is wrongly declared as an enum, every addition is a breaking change; if a closed set is wrongly declared as a class, consumers lose compile-time exhaustiveness for switch expressions, but switch statements are still covered by the exhaustive_cases lint as long as the class has only private constructors. The criterion in the RFC, "whether consumers must handle every value exhaustively, rather than whether new values might be added later", doesn't work as a design rule because it's circular: how consumers handle the values is determined by our choice. If we declare an enum, the tools push developers toward exhaustive switches; if we use a class, switch expressions require a fallback. dart-lang/sdk#63678 also discusses a related criterion (comment): whether each value selects a distinct, non-interchangeable behavior, so that an unknown value can't be handled meaningfully. I don't think that works as a type-level criterion either, since it depends on the use site rather than the type. Keyboard keys each mean something different, yet the set is clearly open and most consumers simply ignore keys they don't handle. Whether a set is open is the only property that belongs to the type itself. The pattern itselfI don't think a private constructor should be mandatory. The cited precedents show why: LogicalKeyboardKey has a public constructor because hardware can produce key IDs Flutter doesn't know about, and TextInputType.numberWithOptions lets callers combine options. Both are legitimate designs. A rough guide could be: use a public constructor when values outside the predefined set are meaningful on their own (arbitrary key IDs), and a private one when a value only makes sense if Flutter supports it (a user-created TargetPlatform would be meaningless). But this should be left to the API author case by case, and the guideline should describe both variants. Note that this choice affects tooling: the exhaustive_cases lint only treats a class as enum-like if all its constructors are private, so with a public constructor, switch statements no longer get the missing-case check. dart-lang/sdk#63678 also discusses alternatives the RFC should evaluate, such as an abstract final class implementing Enum backed by a private enum (comment), which keeps Enum-based APIs working. It also covers a pitfall of the plain pattern: const canonicalization collapses values that don't carry distinct state, so the guideline should require a distinguishing field such as name. Minor: adding a static const to an enum-like class isn't a compile-time break, but with a private constructor, exhaustive_cases (in the recommended lint set) will flag switch statements that don't cover the new constant. That's comparable to a deprecation warning and shouldn't need the breaking change process, but a sentence in the guideline would help. ScopeI'd suggest keeping this RFC scoped to a best-practice recommendation for new APIs, and leaving migration of existing enums to separate, case-by-case discussions (one natural opportunity is when a value needs to be added anyway, since that's already breaking). |
Sorry, something went wrong.
I consider more private files are probably better, if we uses it when it fits. If we force everything to be public, we run into two problems
At least with "@internal" people will be warned to stay away from them.
I don't think so, for g3, they will get warning when they try to use the class, it is not any worse than if anything is forced to be public. for customer_tests, we can reject tests that use direct import. I think we will deal with breaking changes less.
I don't think so , they are currently all feature flagged based and desktop window related.
we can add lint to flutter_lint if they are not already in there.
system like Focus Management exposed a bunch of stuff like FocusAttachment |
Sorry, something went wrong.
|
Hi @dkwingsmt I only talked about visibleForTesting and experimental in open question because I think they serve a slightly different purposes and I am not too sure whether we want to adopt/change them in styleguide. just put it out in open question to see if the discussion lead us toward that direction. will revise the doc a bit based on suggestion |
Sorry, something went wrong.
| * Provides compile-time exhaustiveness checking. | ||
| * Adding a value to a public `enum` is a breaking change and requires to go through the breaking change process. | ||
|
|
||
| ##### Enum-like class with `static const` values (non-exhaustive) |
There was a problem hiding this comment.
A guidance we should bubble up: your enum-like class must have at least one non-private constructor.
If you forget this, Dart's recommended lint exhaustive_cases will enforce exhaustive checks on your enum-like class: https://dart.dev/tools/linter-rules/exhaustive_cases
Sorry, something went wrong.
| - https://github.com/chunhtai | ||
| --- | ||
|
|
||
| # RFC 710.0000: Style guide changes for package private and enums |
There was a problem hiding this comment.
nit: sealed classes are as closed / frozen as enums, what about them?
Sorry, something went wrong.
|
|
||
| #### Placement rules | ||
|
|
||
| * `@internal` may be applied to top-level declarations and to `static` members or constructors of public classes inside `lib/src/`. |
There was a problem hiding this comment.
Loic talked about keeping the rules simple / easy to digest and keeping the complicated details in lint rules. It looks pretty doable to turn these rules into an analyzer plugin lint, instead of adding to the style guide (and/or using a simpler version of this in the style guide).
Sorry, something went wrong.
There was a problem hiding this comment.
Same with the next section.
Sorry, something went wrong.
| Use a standard `enum` when consumers are expected to handle every case without a `default` or `_` branch (such as `Axis`, `AxisDirection`, `VerticalDirection`, and `GrowthDirection`). | ||
|
|
||
| * Provides compile-time exhaustiveness checking. | ||
| * Adding a value to a public `enum` is a breaking change and requires to go through the breaking change process. |
There was a problem hiding this comment.
nit: is a breaking change -> can be a breaking change, as those changes are not necessarily breaking according to our breaking change policy.
Sorry, something went wrong.
There was a problem hiding this comment.
There is a big grey area when the customer_test is not broken. It is that we have to use our judgement to decide whether we "think" this will break people. People don't want to treat new enum as breakage, but when combining with exhaustive_cases it almost guaranteed someone will break.
Sorry, something went wrong.
|
|
||
| ### Removing discouragement of `@visibleForTesting` | ||
|
|
||
| The style guide section "Avoid using `@visibleForTesting`" advises against `@visibleForTesting` on public declarations, preferring APIs testable through their public interfaces, though contributors still use it when necessary. Should the style guide remove its discouragement of `@visibleForTesting` or keep the current stance? |
There was a problem hiding this comment.
The @visibleForTesting annotation marks a public API so that developers that have not disabled the invalid_use_of_visible_for_testing_member analyzer error get a warning when they use this API outside of a test directory.
This means that the API has to be treated as being public (nothing prevents a developer from using the API even in non-test code), meaning it must be designed to be a public API, it must be documented, it must be tested, etc. At which point, there's really no reason not to just make it a public API. If anything, the use of @visibleForTesting becomes merely a crutch to convince ourselves that it's ok that we're making something public that we should really not have made public.
So rather than rely on @visibleForTesting, consider designing your APIs so that they are directly testable using the public API, without exposing any sensitive internals.
(One exception is combining @visibleForTesting with @protected. The @protected annotation marks a member as one that is intended for subclasses, so it is already a public API and considered as such. The @visibleForTesting annotation in that case merely enables the member to be called directly in tests without having to create a fake subclass and without having to add //ignore pragmas.)
The existing guide seems pretty reasonable especially with the @protected exception (the member probably should be @nonVirtual too), and it doesn't say it's strictly forbidden?
Sorry, something went wrong.
There was a problem hiding this comment.
This looks good to me overall!
(It looks like the RFC review process asks for a design review meeting before GitHub approval)
Sorry, something went wrong.
|
|
||
| ## Context | ||
|
|
||
| The style guide discourages package-private APIs beyond library-private (`_`) declarations and requires every file in `lib/src/<layer>/` to be exported by `lib/<layer>.dart`. When an implementation detail is needed across multiple files within a layer, contributors are left with two poor options: |
There was a problem hiding this comment.
Do we have examples where implementation details / secret states have to be exposed within a layer but not to other layers? The examples listed in the next section don't seem super convincing IMO (see the comment below).
Sorry, something went wrong.
There was a problem hiding this comment.
Also flutter/flutter#193960 seems to say another motivation for using @internal is to indicate an API is experimental.
Sorry, something went wrong.
|
Looks like people are generally ok with private files that are under feature flags or cross platform code to use @internal and not exported in barrel export. I think we still need to decide whether it is ok to have package private to hide internal facing class that are not of the above two categories. Looks like in flutter/packages does this a lot, but flutter/flutter is still following the rule as best as it can. Do we want to go ahead and make lift this restriction? I vote for yes, as is in this proposal, WDYT @loic-sharma @justinmc @dkwingsmt @LongCatIsLooong |
Sorry, something went wrong.
There was a problem hiding this comment.
Do you have any specific reasons in mind where you want to start using @internal, or is the motivation for this just the existing violations of the styleguide?
Looks like people are generally ok with private files that are under feature flags or cross platform code to use @internal and not exported in barrel export.
What do you have in mind when you say "cross platform code" here? Or do you mean cross-layer?
Looks like in flutter/packages does this a lot
One example is NoOpPage in go_router, which is probably a good usage.
Sorry, something went wrong.
| #### Enforcement | ||
|
|
||
| * Accidental public exports of `@internal` declarations are caught by the analyzer's [`invalid_export_of_internal_element`](https://dart.dev/tools/diagnostics/invalid_export_of_internal_element) diagnostic, and external usage is caught by [`invalid_use_of_internal_member`](https://dart.dev/tools/diagnostics/invalid_use_of_internal_member) and `implementation_imports`. | ||
| * Repository-specific rules (requiring unexported `lib/src/` files to use a `_` prefix and annotate public top-level declarations with `@internal`, and enforcing instance-member placement rules) will be enforced via custom analyzer lint rules. |
There was a problem hiding this comment.
+1 to automatic enforcement via analyzer.
Overall I think the recommendation in the styleguide is useful as a guideline, but clearly we don't always follow it. I'm supportive of keeping the general guideline but being open to using @internal when it's worth it, especially if we have automated enforcement.
Sorry, something went wrong.
yes, NavigatorState.finalizeRoute and FocusTraversalPolicy. invalidateScopeData are some example that I would have use @internal. They are clean up method are only relevant if you know the internal implementation. People only uses them if they only take a part of the subsystem and build on top. I get that may be use case for these method being public, but we are trading maintainability for this use case.
I meant cases like conditional import for web vs io, there are a bunch for example like this one https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/_web_image_web.dart |
Sorry, something went wrong.
There was a problem hiding this comment.
Is there a different motivating example for introducing @internal? I don't feel the 2 focus related examples are particularly good candidates for @internal.
Sorry, something went wrong.
|
|
||
| * In any file in `lib/src/` not exported by a barrel file, all public top-level declarations must be annotated with `@internal`, subject to the cross-platform API policy below. | ||
|
|
||
| #### Cross-platform APIs |
There was a problem hiding this comment.
cc @loic-sharma and @justinmc , I added this section let me know what you think
Sorry, something went wrong.
|
|
||
| ### Package-private APIs (`@internal`) | ||
|
|
||
| Package-private APIs remain discouraged in general, rather than banned. Unexported files and package-private declarations in `lib/src/` are permitted only in the following cases: |
There was a problem hiding this comment.
I updated the wording to discourage use of package private instead of banning.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Proposing change to style guide around package private in public APIs
Tracking issue: flutter/flutter#193960
This proposal directly contradicts flutter/flutter#108632
Pre-launch Checklist
If you need help, consider asking for advice on the #hackers channel on Discord.