Skip to content

Lint for AnimatedBuilder/ValueListenableBuilder's child property #58551

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

Open
dnfield opened this issue Oct 14, 2021 · 7 comments
Open

Lint for AnimatedBuilder/ValueListenableBuilder's child property #58551

dnfield opened this issue Oct 14, 2021 · 7 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-lint-request P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@dnfield
Copy link
Contributor

dnfield commented Oct 14, 2021

BAD:

return AnimatedBuilder(
  animation: someAnimation,
  builder: _builder,
  // no child
);

Widget _builder(BuildContext context, Widget? child) {
  return SizedBox(
    height: someAnimation.value * someOtherValue,
    child: SomeOtherWidget(), // child does not need the value of the animation
  );
}

GOOD:

return AnimatedBuilder(
  animation: someAnimation,
  builder: _builder,
  child: SomeOtherWidget(),
);

Widget _builder(BuildContext context, Widget? child) {
  return SizedBox(
    height: someAnimation.value * someOtherValue,
    child: child,
  );
}

Similar for ValueListenableBuilder, but that one might be easier because the actual value we're interested in is part of the method signature (so no need to correlate someAnimation as in the AnimatedBuilder).

/cc @Hixie

@dnfield dnfield added type-enhancement A request for a change that isn't a bug linter-lint-request labels Oct 14, 2021
@Hixie
Copy link
Contributor

Hixie commented Oct 14, 2021

Yeah the AnimatedBuilder case is particularly tricky because you have no way to know what the incoming Listenable is actually telling you. It could be "the time has changed" and somewhere else you use DateTime.now(). Or "the network updated the data model" and the rest of your tree is based on reading the data model.

The ValueListenableBuilder one is great though, that one should be much easier to do.

@pq
Copy link
Member

pq commented Oct 15, 2021

Does an annotation on child seem like the right approach here?

const ValueListenableBuilder({
  ...
  @notNull
  this.child,
});

See also: flutter/flutter#91616

@Hixie
Copy link
Contributor

Hixie commented Oct 15, 2021

It's common to not have a child.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 15, 2021

This one is a bit different. It's saying you should use child if there's some widget subtree in the builder function that isn't using the value, e.g.

// a ValueWidgetbuilder
Widget buildWidget(BuildContext context, double value, Widget child) {
  return SizedBox(
    height: value, 
    child: ColoredBox(color: Colors.red),
  );
}

The idea is the analyzer could possibly detect that value is not used in the ColoredBox or below, and tell the user that the ColoredBox should be passed as the child argument to the ValueListenableBuilder that's using buildWidget.

@pq
Copy link
Member

pq commented Oct 15, 2021

Ah, OK. Thanks! This is useful.

Can you think of a way that we could generalize this or communicate this contract via an annotation (or pragma)?

Respecting @Hixie's #58543, I'd like to strive for a solution that is more durable than one that encodes class names and URIs...

@dnfield
Copy link
Contributor Author

dnfield commented Oct 15, 2021

Perhaps something on ValueWidgetBuilder like this:

@widgetTreeBelowLastUsage(variableName: 'value', shouldUse: 'child')
typedef ValueWidgetBuilder<T> = Widget Function(BuildContext context, T value, Widget? child);

@dnfield
Copy link
Contributor Author

dnfield commented Oct 15, 2021

(I'm not fond of that name but you get the idea hopefully)

@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 19, 2024
@pq pq added the P3 A lower priority bug or feature request label Nov 20, 2024
@bwilkerson bwilkerson added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-lint-request P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants