This repository was archived by the owner on Nov 20, 2024. It is now read-only.
feat: lint when widgets are being returned outside the build method #2582
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgecoca
changed the title
Kirpal
reviewed
Apr 13, 2021
|
/fyi @Hixie @goderbauer @jacob314 @a14n for input on semantics. |
bwilkerson
reviewed
Apr 13, 2021
pq
reviewed
Apr 13, 2021
a14n
reviewed
Apr 14, 2021
bwilkerson
approved these changes
Apr 14, 2021
| bool _isBuildMethod(ExecutableElement? element) => element?.name == 'build'; | ||
|
|
||
| bool _isBuildMethodOfWidget(MethodDeclaration node) { | ||
| if (_isBuildMethod(node.declaredElement)) { |
There was a problem hiding this comment.
This is fine as-is, but I'll point out that you don't need to access the ExecutableElement in order to get the name of the method. You can access it as node.name.name and doing so avoids the need for a null-aware operator in _isBuildMethod.
There was a problem hiding this comment.
Ha! Things I've learned today! Is it just improving the syntax, or are there performance improvements as well?
There was a problem hiding this comment.
The null check takes such a small amount of time that I doubt it would be noticable. The fact that the information is always available is the only advantage, and even that's negligible because I don't think the declaredElement can every be null in practice in the linter code. Mostly just passing along information in case it's useful some day.
felangel
reviewed
Apr 14, 2021
felangel
reviewed
Apr 14, 2021
felangel
reviewed
Apr 14, 2021
felangel
reviewed
Apr 14, 2021
felangel
reviewed
Apr 14, 2021
|
I really like the overall idea of using lints to guide users to using Widgets rather than functions where possible. This could be a good candidate for a hypothetical level of analyzer hints that only showed up when you authored code. That way a users could be aware of the best practice but you wouldn't have to worry as much about false positives. Fyi @davidmorgan. And false positives from Flutter. |
|
Even without the I noticed that some of them are because I also noticed that many of them are private methods/functions. I wonder whether the lint should ignore those (and local functions). It seems like the advice to use a class is more important when you're trying to share the widget structure in multiple places that when you just trying to break up a large |
To be clear. I used a regexp instead of the lint to get the quick list of false positives. I assume the actual lint wouldn't be tripped up by the function type cases. However, each of the function types does however give an example where users need to write a function instead of a Widget class. |
|
I am not sure if banning these outright is a good idea. There are legit use cases where you may want to return a widget from a non-build method, as Jacob also pointed out above. The most prominent ones are builder methods. Simple example: import 'package:flutter/widgets.dart';
class Foo extends StatelessWidget {
Widget _buildMyWidget(BuildContext context) { // Should not lint.
return Container();
}
@override
Widget build(BuildContext context) {
return Builder(
builder: _buildMyWidget,
);
}
}Not sure how one would differentiate this from other methods? |
Oh yeah! That's a great example, and I did not cover that in tests. Let me see if there's anything it could be done. Thank you for the feedback! |
|
Here is how you can solve the builder case robustly. You need to change where you are showing the lint error. Rather that showing a lint error when you define a method that returns a Widget, show a lint error when you invoke a method that returns a Widget directly in a build method. class Foo extends StatelessWidget {
Widget _buildMyWidget(BuildContext context) { // Should not lint.
return Container();
}
@override
Widget build(BuildContext context) {
return _buildMyWidget(context); // LINT ERROR
}
}while class Foo extends StatelessWidget {
Widget _buildMyWidget(BuildContext context) { // Should not lint.
return Container();
}
@override
Widget build(BuildContext context) {
return Builder(
builder: _buildMyWidget, // OK
);
}
} |
|
I agree with @goderbauer that banning these outright is not a good idea. Certainly it's usually better to use a widget rather than a method to build widgets, but that's more of a guideline than a hard-and-fast rule. Like @jacob314 says, it might be better as a clippy-style suggestion during development ("would you like to extract this out into a widget class?") than a lint. |
|
Closing for now. I will try to figure out a better way to handle these cases. Thank you all so much for the feedback! |
|
Thanks so much for motivating such a thoughtful conversation. I'm sure I speak for everyone when I see I look forward to seeing what you come up with! |
|
Allowing them when they're declared as anonymous functions could solve the builder issue, forcing the functional widget to become a class widget or be part of the code tree. This one goes against @goderbauer's comment, but I think that forcing it into the build tree or in its own class FunctionalBuilder extends StatelessWidget {
const FunctionalBuilder({Key key}) : super(key: key);
@override
Widget build(BuildContext context) {
return Builder(
builder: _myWidget,
);
}
/// Bad
Widget _myWidget(BuildContext context) {
return Container();
}
}class AnonymousBuilder extends StatelessWidget {
const AnonymousBuilder({Key key}) : super(key: key);
@override
Widget build(BuildContext context) {
return Builder(
builder: (BuildContext context) {
return const MyWidget();
},
);
}
}
/// Good
class MyWidget extends StatelessWidget {
const MyWidget({Key key}) : super(key: key);
@override
Widget build(BuildContext context) {
return Container();
}
}Another notable exception is when they're defined but not declared/instantiated. /// Good
typedef WidgetBuilder = Widget Function(BuildContext context);
class Builder extends StatelessWidget {
const Builder({
Key key,
@required this.builder,
}) : assert(builder != null),
super(key: key);
/// Good
final WidgetBuilder builder;
/// Exception because of `@override`
@override
Widget build(BuildContext context) {
return builder(context);
}
}These two exceptions, combined with an exception on It's important to point out that @jacob314's list of false positives is misleading; many cases are actual positives that should actually be disallowed. For example, have a look at I think @jacob314's solution is a great example of out-of-the-box thinking, it's more flexible but still satisfying. After reading this discussion, I edited dart-lang/sdk#58303 to add a few more allow-cases / disallow-cases. I don't have the expertise that many of you have, so I hope you can point out use cases that I missed. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One of the most common questions I have received is why it is preferred to use classes to create widgets in Flutter, as opposed to returning and reusing widgets from fields and methods.
This SO issue explains really well the issue:
https://stackoverflow.com/questions/53234825/what-is-the-difference-between-functions-and-classes-to-create-reusable-widgets
With this new lint rule, I attempt to highlight cases where a widget is not created properly.
Fixes
dart-lang/sdk#58303