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

Problem with sharing Hook values between HookWidget class's methods #120

Open
sahandevs opened this issue Jan 9, 2020 · 13 comments
Open
Assignees

Comments

@sahandevs
Copy link
Contributor

one common action in flutter is to extract widget into functions when it gets very large:

from:

    return Scaffold(
      body: Column(
        children: <Widget>[
          // header,
          // content
          // footer
        ],  
      ),
    );

to:

    return Scaffold(
      body: Column(
        children: <Widget>[
          buildHeader(),
          buildContent(),
          buildFooter(),
        ],
      ),
    );

if we do this in a Hook Widget we must add parameters for the generated functions because values are inside build function and are not properties so they are inaccessible. It gets annoying quickly. (specially for something like useTheme).

we can use hooks inside extracted methods too but it may cause bugs later because we made that part of widget conditional.

one solution is to create a wrapper widget :

class MyWidget extends HookWidget {
  @override
  Widget build(BuildContext context) {
    final text = useState("My Text");
    return _MyWidget(
      text: text,
    );
  }

}

class _MyWidget extends StatelessWidget {

  final ValueNotifier<String> text;

  const _MyWidget({Key key, this.text}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Container(
      child: Text(text.value),
    );
  }
}

doing so will fix the problem above but:

  • A lot of boilerplate code involved
  • Some hooks return classes which you need to import in order to pass it into _MyWidget
  • Some hooks return Private classes which you can't do anything about them.

another solution is to use local functions with closure inside HookWidget:

class MyWidget extends HookWidget {
  
  @override
  Widget build(BuildContext context) {
    final text = useState("My Text");
    
    Widget buildText() {
      return Text(text.value);
    }
    
    return Container(
      child: buildText(),
    );
  }

}

It solves the above problem and makes it really easy to use (like ReactJS's hooks). and I don't think performance is a problem here.

problems (challenges) with this approach is:

  • we may need a hook for callbacks: useCallBack
  • Extract method in Android Studio or VSCode does not extract to local functions
  • more indentation (though I don't think is a problem)

we can also create a widget like InlineHookWidget which takes a builder and acts like above:

class MyWidget extends HookWidget {

  @override
  Widget build(BuildContext context) {
    final text = useState("My Text");

    Widget buildText() {
      return InlineHookWidget((context) {
          final counter = useState(0);  
          return Text(counter.value.text);
      });
    }

    return Container(
      child: buildText(),
    );
  }

}

with this we can use hooks in loops or very small widgets that needs state but creating whole new separate HookWidget is not reasonable (or for fast prototyping)

@rrousselGit
Copy link
Owner

InlineHookWidget == HookBuilder

Consider taking a look at functional_widget

And I don't quite understand what's the question/proposal

@sahandevs
Copy link
Contributor Author

sahandevs commented Jan 10, 2020

@rrousselGit sorry I wasn't clear
It is a suggestion to add documents about recommended ways to use hooks for newcomers to avoid the problems above

@fgblomqvist
Copy link

I had similar thoughts as @sahandevs, since I like the idea of splitting up the build function (which is seemingly very common). I suppose that React essentially uses local functions, so perhaps that's the approach that should be used/recommended. My main concern with that would be the performance impact since all functions would get re-instantiated on every build. Not an expert on Dart so not sure what the consensus on that stuff is? The InlineHookWidget/HookBuilder stuff seems to solve a different kind of problem so not too sure about that.

Nonetheless, I do agree that a paragraph and an example should be added somewhere (e.g. readme) regarding how flutter_hooks can be used in a larger app (where you need/want to split your code up a bit more).

@rrousselGit
Copy link
Owner

Will do.

I am finishing a project and then I will be working on giving hooks some attention (after fixing the build of functional_widget)

@fgblomqvist
Copy link

Sounds good, appreciate the quick answer!
Now that I think about it some more, maybe it does make more sense to just pass the hook values (i.e. stay functional). The import of the classes is not the end of the world, but I'm not sure which hooks would return private classes (seems kinda counter-intuitive)? Furthermore, you'd only need to pass the entire ValueNotifier if the function needs to change the value (i.e. modify state).

Here's some examples of how code could be written (depending on whether you prefer local functions or not):

class MyWidget extends HookWidget {
  @override
  Widget build(BuildContext context) {
    final text = useState('My Text');

    void modifyStringLocal() {
      s.value += ' but it might rain later.';
    }

    text.value = makeString();
    modifyString(text);
    modifyStringLocal();

    return Container(
      child: buildText(text.value),
    );
  }

  Widget buildText(String text) {
    return Text(text);
  }

  String makeString() {
    return 'Hello world';
  }

  void modifyString(ValueNotifier<String> s) {
    s.value += ' it is sunny outside';
  }
}

@esDotDev
Copy link

esDotDev commented Dec 21, 2020

This is definitely one of the main downsides with Hooks in Flutter, you lose that really nice pattern you can do with a class-based widget which allows you to split your tree into multiple widgets, with virtually no boilerplate. Essentially a quick and dirty ViewModel pattern that can handle multiple views, which is quite nice actually, especially for responsive designs where we might want to swap out views but keep the same controller in place.

class StatefulView extends StatefulWidget {
  const StatefulView({Key key, this.foo, this.bar}) : super(key: key);
  final bool foo;
  final int bar;
  @override
  _StatefulViewState createState() => _StatefulViewState();
}

class _StatefulViewState extends State<StatefulView> {
  TextEditingController _controller1;
  ScrollController _controller2;

  void _handleAction1() {}
  void _handleAction2() {}
  void _calculateSomething() {}

  @override
  Widget build(BuildContext context) {
    return Row(
      children: [
        // These views can access any properties on Widget, or method in the state.
        _SubView1(this),
        _SubView2(this),
        _SubView3(this),
      ],
    );
  }
}

@rrousselGit
Copy link
Owner

Passing the State around is a very bad practice.

This goes against separation of concern where widgets should be independent, and the only purpose is reducing boilerplate.

Instead have your subviews receive callbacks and parameters:

_Subview1(
  controller1: controller1,
  _handleAction1: () {}
)

@esDotDev
Copy link

esDotDev commented Dec 22, 2020

No, that's much too dogmatic of a way of looking at it imo, things aren't black and white like this. If that widget has a reason to be known to the outside app, ie, it is re-used, it is violation separation of concerns I agree. If the widget is used only internally, and that implementation is opaque to the rest of the application, then it really is not. If you need to use something in multiple views, then of course it should follow the classic pattern of callbacks and params.

It's an extremely common architectural pattern, to have a view that is layout, tied to a controller with some API. It leads to much less repetition and cleaner code overall. I don't see any harm using this internally in some way, it allows 'hot-swapping' of views extremely quickly. This is really no different than a view looking up to a model for some business logic, here we have multiple views looking up to a shared state, that they receive as a direct injection.

Lets say I have some view, like ClockPage(). It's stateful, and it has 5 or 6 controller methods, and then also various internal views _PhoneView _WatchView, _DesktopView etc. If I pass state directly, all the views can use it as a shared controller, boilerplate is minimal, nothing is repeated. The views are free to make the calls they need to make. The state knows nothing about who is triggering the ui calls, it just decides which views to show. You can end up with a very nice layout here, where you have 5 classes in 1 file, 150 lines, and a super clean seperation of business logic and layout code. Where's the problem? We can also easily change layouts completely, without losing state between them, that's a really nice freebie.

If I do it as you are mentioning, each time I change the API I have 3, 4, 5 changes to make. I gain as many as 5-10 lines per view. And it's really accomplishing nothing because I simply never use these views in a standalone way. This is simply an argument about coupled vs de-coupling, and the position that de-coupled is always better is not fully thought through. De-coupling has downsides as well.

If for some reason in the future, it turns out WatchView is useful elsewhere, it's a 5m job to de-couple from the hard dependencies on state, since it's used as a pure controller anyways.

I don't use this a ton, but it can be very useful when you are only deconstructing a tree for readability, and the goal is to actually not separate concerns, but rather just to organize the code a little better.

The idea that you can only go from one big giant function, to classes with a ton of boilerplate interfaces, and nothing in the middle , doesn't ring true for me. Somehow it's ok to share state over 400 lines of code, with tons of nested build-methods, but passing around a clean pointer, to some parent state, to multiple stateless views (which are basically just build() calls) is "very bad practice"?

For example, we we arguing there is a large difference here in "good practice" between:

bool _sharedState;
AnimationController _sharedController;
void sharedMethod1(){};
void sharedMethod2(){};

void build(){
  Widget buildSubView1(){ ... }
  Widget buildSubView2(){ ... }
  return Row(widget.largeMode?  buildSubView1() : buildSubView2());
}

And:

bool _sharedState;
AnimationController _sharedController;
void sharedMethod1(){};
void sharedMethod2(){};

void build(){
  return Row(widget.largeMode?  _SubView1(this) : _SubView2(this));
}


class _SubView1 extends StatelessWidget {
   const _SubView1 (this.state);
   final _ViewState state;

   Widget build(BuildContext context) { ... }
}

class _SubView2 {
   const _SubView2 (this.state);
   final _ViewState state;

   Widget build(BuildContext context) { ... }
}

This seems like a stylistic difference to me?

@esDotDev
Copy link

esDotDev commented Dec 22, 2020

This is the same sort of thinking that leads C# developers to thing every single object needs an interface. If you have a reason to de-couple, de-couple by all means. It's a tool to solve a problem, a means to an end, not an end in itself. Thinking that every single thing you ever build needs to plan for future use-cases that never happen, is just pointless and leads to bugs and rigidity,

@esDotDev
Copy link

esDotDev commented Dec 22, 2020

The end result here is the same, buildSubView1() has full access to all the ViewState, as does _SubView1(). The only difference is one is giant functional blob, and the other has some clear markers around the view code, and separates it from the button handlers and fields. ViewState can not directly access anything in the SubView. Identical in both cases.

Not trying to be argumentative, I've just heard this repeated multiple times, and trying to get to the bottom of it. This seems to be something people repeat, but if you dig into the rationale, there's nothing there and they don't know why they are repeating it.

@adam-ashored
Copy link

adam-ashored commented Dec 24, 2020

Somehow it's ok to share state over 400 lines of code, with tons of nested build-methods, but passing around a clean pointer, to some parent state, to multiple stateless views (which are basically just build() calls) is "very bad practice"?

@esDotDev is 100% correct. The "dogmatic" way is actually a bad practice. Personally, I'd even go so far as to suggest that it should be considered an anti-pattern. Multiple stateless widgets creates a much smaller API surface for your widget, much more easy to test - testing multiple single responsibility widgets is way easier than testing a single god widget.

The React community suffered from this extracting rendering things into functions as well (it arguably still does). Instead of breaking down your build method into class functions, break it down into smaller widgets.

The side-effect that I've seen is that eventually, devs realize they're creating the same (or similar) small widgets used by different larger ones, so you can create a single widget that can be shared. And when you do that, you've won composable interfaces, because that's how they're supposed to work.

@praxder
Copy link

praxder commented Oct 20, 2022

Any update on the recommended way to utilize useState if you're using builder functions to split up a large build method?

@rrousselGit rrousselGit self-assigned this May 10, 2023
@Coinners
Copy link

hey @rrousselGit is there a recommended way to deal with this problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants