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

proposal: Consider adding new avoid_shrink_wrap_in_lists lint #3496

Open
gabrielaraujoz opened this issue Jul 7, 2022 · 5 comments
Open
Labels
lint-request type-enhancement A request for a change that isn't a bug

Comments

@gabrielaraujoz
Copy link

gabrielaraujoz commented Jul 7, 2022

Describe the rule you'd like to see implemented
This would be a nice lint for Flutter.

According to the Flutter documentation, using shrinkWrap in lists is expensive performance-wise and should be avoided, since using slivers is significantly cheaper and achieves the same or even better results.

ShrinkWrap is usually needed when you have a list inside another list, or a list that should not be scrollable inside a Column or roll. From the docs, it is described as: Whether the extent of the scroll view in the scrollDirection should be determined by the contents being viewed.

I then would like to propose that the team considers adding this performance lint to Dart/Flutter first as optional, but it could eventually even be recommended since it helps with Flutter's performance directly and it's use case is not uncommon.

It could ignore if a list is not contained inside another list, column or row (in this case, slivers would not be needed anyways).

Additional context
Flutter shrinkWrap doc link - Video explaining the difference and performance disparity

Bad Example

Column(
      children: [
        Expanded(
          child: ListView(
            shrinkWrap: true,
            physics: const NeverScrollableScrollPhysics(),
            children: [],
            ),
       ),
  ],
),

Good Example

CustomScrollView(
  slivers: [
    SliverList(
      delegate: SliverChildBuilderDelegate(
        (context, index) {
          return Container();
        },
        childCount: someObject.length,
      ),
    ),
  ],
),
@gabrielaraujoz gabrielaraujoz added type-enhancement A request for a change that isn't a bug lint-request labels Jul 7, 2022
@gabrielaraujoz gabrielaraujoz changed the title Consider adding new avoid_shrink_wrap_in_lists lint proposal: Consider adding new avoid_shrink_wrap_in_lists lint Jul 7, 2022
@bwilkerson
Copy link
Member

@goderbauer

@goderbauer
Copy link
Contributor

@dnfield @Piinks Did we write down some learnings from our recent discussion on this somewhere? I seem to remember we identified some valid use cases for shrink wrap, which may make a general "avoid shrink wrap" lint a little too broad. However, if we can instead target certain known bad use cases with it (like using shrink wrap to place a list inside another list), this could be helpful.

@dnfield
Copy link

dnfield commented Jul 7, 2022

@douglasiacovelli
Copy link

douglasiacovelli commented Jul 7, 2022

Even though there are valid use cases, shrinkWrap is not recommended because of performance impact when it's easily avoidable with SliverLists. I wouldn't see an issue to add an ignore comment in case it is really needed so we are at least well aware of it. Also it could be more of a warning than an error itself.

@gabrielaraujoz
Copy link
Author

@goderbauer I totally agree with you there, this could be pretty useful for performance house-keeping if we focus on the known bad use cases! nowadays when we face this problem, shrinkwrapping is the easiest way out instead of changing your widget tree to use slivers. I see we have already removed the suggestion for using shrinkWrap when we get the unbounded height/width UI errors, maybe we could then suggest the use of slivers in this case as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint-request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants