flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
162.54k stars 26.72k forks source link

Reusing state logic is either too verbose or too difficult #51752

Open rrousselGit opened 4 years ago

rrousselGit commented 4 years ago

.

Related to the discussion around hooks #25280

TL;DR: It is difficult to reuse State logic. We either end up with a complex and deeply nested build method or have to copy-paste the logic across multiple widgets.

It is neither possible to reuse such logic through mixins nor functions.

Problem

Reusing a State logic across multiple StatefulWidget is very difficult, as soon as that logic relies on multiple life-cycles.

A typical example would be the logic of creating a TextEditingController (but also AnimationController, implicit animations, and many more). That logic consists of multiple steps:

This, in itself, is not complex. The problem starts when we want to scale that approach. A typical Flutter app may have dozens of text-fields, which means this logic is duplicated multiple times.

Copy-pasting this logic everywhere "works", but creates a weakness in our code:

The Mixin issue

The first attempt at factorizing this logic would be to use a mixin:

mixin TextEditingControllerMixin<T extends StatefulWidget> on State<T> {
  TextEditingController get textEditingController => _textEditingController;
  TextEditingController _textEditingController;

  @override
  void initState() {
    super.initState();
    _textEditingController = TextEditingController();
  }

  @override
  void dispose() {
    _textEditingController.dispose();
    super.dispose();
  }

  @override
  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
    super.debugFillProperties(properties);
    properties.add(DiagnosticsProperty('textEditingController', textEditingController));
  }
}

Then used this way:

class Example extends StatefulWidget {
  @override
  _ExampleState createState() => _ExampleState();
}

class _ExampleState extends State<Example>
    with TextEditingControllerMixin<Example> {
  @override
  Widget build(BuildContext context) {
    return TextField(
      controller: textEditingController,
    );
  }
}

But this has different flaws:

This makes mixins both un-ideal and too dangerous to be a true solution.

Using the "builder" pattern

Another solution may be to use the same pattern as StreamBuilder & co.

We can make a TextEditingControllerBuilder widget, which manages that controller. Then our build method can use it freely.

Such a widget would be usually implemented this way:

class TextEditingControllerBuilder extends StatefulWidget {
  const TextEditingControllerBuilder({Key key, this.builder}) : super(key: key);

  final Widget Function(BuildContext, TextEditingController) builder;

  @override
  _TextEditingControllerBuilderState createState() =>
      _TextEditingControllerBuilderState();
}

class _TextEditingControllerBuilderState
    extends State<TextEditingControllerBuilder> {
  TextEditingController textEditingController;

  @override
  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
    super.debugFillProperties(properties);
    properties.add(
        DiagnosticsProperty('textEditingController', textEditingController));
  }

  @override
  void dispose() {
    textEditingController.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return widget.builder(context, textEditingController);
  }
}

Then used as such:

class Example extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return TextEditingControllerBuilder(
      builder: (context, controller) {
        return TextField(
          controller: controller,
        );
      },
    );
  }
}

This solves the issues encountered with mixins. But it creates other issues.

rrousselGit commented 4 years ago

cc @dnfield @Hixie As requested, that's the full details on what are the problems solved by hooks.

dnfield commented 4 years ago

I'm concerned that any attempt to make this easier within the framework will actually hide complexity that users should be thinking about.

It seems like some of this could be made better for library authors if we strongly typed classes that need to be disposed with some kind of abstract class Disposable. In such a case you should be able to more easily write a simpler class like this if you were so inclined:

class AutomaticDisposingState<T> extends State<T> {
  List<Disposable> _disposables;

  void addDisposable(Disposable disposable) {
    assert(!_disposables.contains(disposable));
    _disposables.add(disposable);
  }

  @override
  void dispose() {
    for (final Disposable disposable in _disposables)
      disposable.dispose();
    super.dispose();
  }
}

Which gets rid of a few repeated lines of code. You could write a similar abstract class for debug properties, and even one that combines both. Your init state could end up looking something like:

@override
void initState() {
  super.initState();
  controller = TextEditingController(text: 'Hello world');
  addDisposable(controller);
  addProperty('controller', controller);
}

Are we just missing providing such typing information for disposable classes?

rrousselGit commented 4 years ago

I'm concerned that any attempt to make this easier within the framework will actually hide complexity that users should be thinking about.

Widgets hides the complexity that users have to think about. I'm not sure that's really a problem.

In the end it is up to users to factorize it however they want.


The problem is not just about disposables.

This forgets the update part of the problem. The stage logic could also rely on lifecycles like didChangeDependencies and didUpdateWidget.

Some concrete examples:

rrousselGit commented 4 years ago

There are many examples in the framework where we want to reuse state logic:

These are nothing but a way to reuse state with an update mechanism.

But they suffer from the same issue as those mentioned on the "builder" part.

That causes many problems. For example one of the most common issue on Stackoverflow is people trying to use StreamBuilder for side effects, like "push a route on change".

And ultimately their only solution is to "eject" StreamBuilder. This involves:

That's a lot of work, and it's effectively not reusable.

Hixie commented 3 years ago

Problem

Reusing a State logic across multiple StatefulWidget is very difficult, as soon as that logic relies on multiple life-cycles.

A typical example would be the logic of creating a TextEditingController (but also AnimationController, implicit animations, and many more). That logic consists of multiple steps:

  • defining a variable on State.
    TextEditingController controller;
  • creating the controller (usually inside initState), with potentially a default value:
    @override
    void initState() {
    super.initState();
    controller = TextEditingController(text: 'Hello world');
    }
  • disposed the controller when the State is disposed:
    @override
    void dispose() {
    controller.dispose();
    super.dispose();
    }
  • doing whatever we want with that variable inside build.
  • (optional) expose that property on debugFillProperties:
    void debugFillProperties(DiagnosticPropertiesBuilder properties) {
    super.debugFillProperties(properties);
    properties.add(DiagnosticsProperty('controller', controller));
    }

This, in itself, is not complex. The problem starts when we want to scale that approach. A typical Flutter app may have dozens of text-fields, which means this logic is duplicated multiple times.

Copy-pasting this logic everywhere "works", but creates a weakness in our code:

  • it can be easy to forget to rewrite one of the steps (like forgetting to call dispose)
  • it adds a lot of noise in the code

I really have trouble understanding why this is a problem. I've written plenty of Flutter applications but it really doesn't seem like that much of an issue? Even in the worst case, it's four lines to declare a property, initialize it, dispose it, and report it to the debug data (and really it's usually fewer, because you can usually declare it on the same line you initialize it, apps generally don't need to worry about adding state to the debug properties, and many of these objects don't have state that needs disposing).

I agree that a mixin per property type doesn't work. I agree the builder pattern is no good (it literally uses the same number of lines as the worst case scenario described above).

Hixie commented 3 years ago

With NNBD (specifically with late final so that initiializers can reference this) we'll be able to do something like this:

typedef Initializer<T> = T Function();
typedef Disposer<T> = void Function(T value);

mixin StateHelper<T extends StatefulWidget> on State<T> {
  bool _active = false;
  List<Property<Object>> _properties = <Property<Object>>[];

  @protected
  void registerProperty<T>(Property<T> property) {
    assert(T != Object);
    assert(T != dynamic);
    assert(!_properties.contains(property));
    _properties.add(property);
    if (_active)
      property._initState();
  }

  @override
  void initState() {
    _active = true;
    super.initState();
    for (Property<Object> property in _properties)
      property._initState();
  }

  @override
  void dispose() {
    for (Property<Object> property in _properties)
      property._dispose();
    super.dispose();
    _active = false;
  }

  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
    super.debugFillProperties(properties);
    for (Property<Object> property in _properties)
      property._debugFillProperties(properties);
  }
}

class Property<T> {
  Property(this.owner, this.initializer, this.disposer, [ this.debugName ]) {
    owner.registerProperty(this);
  }

  final StateHelper<StatefulWidget> owner;
  final Initializer<T> initializer;
  final Disposer<T> disposer;
  final String debugName;

  T value;

  void _initState() {
    if (initializer != null)
      value = initializer();
  }

  void _dispose() {
    if (disposer != null)
      disposer(value);
    value = null;
  }

  void _debugFillProperties(DiagnosticPropertiesBuilder properties) {
    properties.add(DiagnosticsProperty(debugName ?? '$T property', value));
  }
}

You'd use it like this:

class MyHomePage extends StatefulWidget {
  MyHomePage({Key key, this.title}) : super(key: key);

  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> with StateHelper<MyHomePage> {
  late final Property<int> _counter = Property<int>(this, null, null);
  late final Property<TextEditingController> _text = Property<TextEditingController>(this,
    () => TextEditingController(text: 'button'),
    (TextEditingController value) => value.dispose(),
  );

  void _incrementCounter() {
    setState(() {
      _counter.value += 1;
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            Text(
              'You have pushed the ${_text.value.text} this many times:',
            ),
            Text(
              '${_counter.value}',
              style: Theme.of(context).textTheme.headline4,
            ),
            TextField(
              controller: _text.value,
            ),
          ],
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: _incrementCounter,
        tooltip: 'Increment',
        child: Icon(Icons.add),
      ),
    );
  }
}

Doesn't seem to really make things better. It's still four lines.

Hixie commented 3 years ago

Widgets hides the complexity that users have to think about.

What do they hide?

rrousselGit commented 3 years ago

The problem is not the number of lines, but what these lines are.

StreamBuilder may be about as many lines as stream.listen + setState + subscription.close. But writing a StreamBuilder can be done without any reflection involved, so to say. There is no mistake possible in the process. It's just "pass the stream, and build widgets out of it".

Whereas writing the code manually involves a lot more thoughts:

rrousselGit commented 3 years ago

What do they hide?

apps generally don't need to worry about adding state to the debug properties

They don't, because they do not want to deal with the complexity of maintaining the debugFillProperties method. But if we told developers "Would you like it is out of the box all of your parameters and state properties were available on Flutter's devtool?" I'm sure they would say yes

Many people have expressed to me their desire for a true equivalent to React's devtool. Flutter's devtool is not yet there. In React, we can see all the state of a widget + its parameters, and edit it, without doing anything.

Similarly, people were quite surprised when I told them that when using provider + some other packages of mine, by default their entire application state is visible to them, without having to do anything (modulo this annoying devtool bug)

Hixie commented 3 years ago

I have to admit that I'm not a big fan of FutureBuilder, it causes a lot of bugs because people don't think about when to trigger the Future. I think it would not be unreasonable for us to drop support for it. StreamBuilder is ok I guess but then I think Streams themselves are too complicated (as you mention in your comment above) so...

Why does someone have to think about the complexity of creating Tweens?

ListView doesn't really hide the logic of mounting a widget as it appears; it's a big part of the API.

The problem is not the number of lines, but what these lines are.

I really don't understand the concern here. The lines seem pretty much like simple boilerplate. Declare the thing, initialize the thing, dispose of the thing. If it's not the number of lines, then what's the problem?

rrousselGit commented 3 years ago

I'll agree with you that FutureBuilder is problematic.

It's a bit off-topic, but I would suggest that in development, Flutter should trigger a fake hot-reload every few seconds. This would highlight misuses of FutureBuilder, keys, and many more.

Why does someone have to think about the complexity of creating Tweens?

ListView doesn't really hide the logic of mounting a widget as it appears; it's a big part of the API.

We agree on that. My point was that we cannot criticize something like hooks with "it hides logic", as what hooks do is strictly equivalent to what a TweenAnimationBuilder/AnimatedContainer/... do. The logic is not hidden

In the end, I think animations are a good comparison. Animations have this concept of implicit vs explicit. Implicit animations are loved because of their simplicity, composability and readability. Explicit animations are more flexible, but more complex.

When we translate this concept to listening to streams, StreamBuilder is an implicit listening, whereas stream.listen is explicit.

More specifically, with StreamBuilder you cannot forget to handle the scenario where the stream changes, or forget to close the subscription. You can also combine multiple StreamBuilder together

stream.listen is slightly more advanced and more error-prone.

Builders are powerful to simplify the application. But as we agreed on previously, the Builder pattern is not ideal. It's both verbose to write and to use. This issue, and what hooks solve, is about an alternate syntax for *Builders

For example, flutter_hooks has a strict equivalent to FutureBuilder and StreamBuilder:

Widget build(context) {
  final AsyncSnapshot<T> snapshot = useStream(stream);
}

In the continuation, AnimatedContainer & alike could be represented by a useAnimatedSize / useAnimatedDecoractedBox / ... such that we have:

double opacity;

Widget build(context) {
  final double animatedOpacity = useAnimatedDouble(opacity, duration: Duration(milliseconds: 200));
  return Opacity(
    opacity: animatedOpacity,
    child: ...,
  );
}
Hixie commented 3 years ago

My point was that we cannot criticize something like hooks with "it hides logic",

That's not the argument. The argument is "it hides logic that developers should be thinking about".

rrousselGit commented 3 years ago

Do you have an example of such logic that the developers should be thinking about?

Hixie commented 3 years ago

Like, who owns the TextEditingController (who creates it, who disposes of it).

rrousselGit commented 3 years ago

Like with this code?

Widget build(context) {
  final controller = useTextEditingController();
  final focusNode = useFocusNode();
}

The hook creates it and disposes of it.

I'm not sure what is unclear about this.

Hixie commented 3 years ago

Yes, exactly. I have no idea what the lifecycle of the controller is with that code. Does it last until the end of the lexical scope? The lifetime of the State? Something else? Who owns it? If I pass it to someone else, can they take ownership? None of this is obvious in the code itself.

rrousselGit commented 3 years ago

It looks like your argument is caused more by a lack of understanding on what hooks do rather than a real issue. These questions have a clearly defined answer that is consistent with all hooks:

I have no idea what the lifecycle of the controller is with that code

Nor do you have to think about it. It is no longer the responsibility of the developer.

Does it last until the end of the lexical scope? The lifetime of the State

The lifetime of the State

Who owns it?

The hook owns the controller. It is part of the API of useTextEditingController that it owns the controller. This applies to useFocusNode, useScrollController, useAnimationController, ...

In a way, these questions apply to StreamBuilder:

rrousselGit commented 3 years ago

In general, you can think of:

final value = useX(argument);

as a strict equivalent to:

XBuilder(
  argument: argument,
  builder: (context, value) {

  },
);

They have the same rules and the same behavior.

Hixie commented 3 years ago

It is no longer the responsibility of the developer

I think fundamentally that's the disagreement here. Having a function-like API that returns a value that has a defined life-time that isn't clear is, IMHO, fundamentally very different than an API based on passing that value to a closure.

I have no problem with someone creating a package that uses this style, but it's a style contrary to the kind that I would want to include in the core flutter API.

emanuel-lundman commented 3 years ago

@Hixie I don't think what @rrousselGit was saying was that they are the same thing but just that they have "the same rules and the same behaviour" regarding life cycle? Correct?

They don't solve the same problems though.

Maybe I'm wrong here but last fall when trying out flutter I believe that if I would have needed three of those builders in one widget it would have been a lot of nesting. Compared to three hooks (three lines). Also. Hooks are composable so if you need to share state logic composed of multiple hooks you could make a new hook that uses other hooks and some extra logic and just use the one new hook.

Such stuff like sharing state logic easily between widgets was a thing I was missing when trying out flutter fall of 2019.

There could of course be a lot of other possible solutions. Maybe it's already been solved and I just didn't find it in the docs. But if not there are a lot that could be done to speed up development a great deal if a thing like hooks or another solution for the same problems where available as a first class citizen.

Hixie commented 3 years ago

I'm definitely not suggesting using the builder approach, as the OP mentions, that has all kinds of problems. What I would suggest is just using initState/dispose. I don't really understand why that's a problem.

I'm curious how people feel about the code in https://github.com/flutter/flutter/issues/51752#issuecomment-664787791. I don't think it's any better than initState/dispose, but if people like hooks, do they like that too? Is hooks better? Worse?

satvikpendem commented 3 years ago

@Hixie Hooks are nice to use because they compartmentalize the life cycle into a single function call. If I use a hook, say useAnimationController, I don't have to think about initState and dispose anymore. It removes the responsibility from the developer. I don't have to worry whether I disposed every single animation controller I created.

initState and dispose are fine for a single thing but imagine having to keep track of multiple and disparate types of state. Hooks compose based on the logical unit of abstraction instead of spreading them out in the life cycle of the class.

I think what you're asking is the equivalent of asking why have functions when we can manually take care of effects every time. I agree it is not exactly the same, but it broadly feels similar. It seems that you have not used hooks before so the problems don't seem too apparent to you, so I would encourage you to do a small or medium size project using hooks, with the flutter_hooks package perhaps, and see how it feels. I say this with all respect, as a user of Flutter I have run into these issues that hooks provide solutions to, as have others. I am unsure how to convince you that these problems truly exist for us, let us know if there's a better way.

gaearon commented 3 years ago

I'll add a few thoughts from the React perspective. Pardon if they're not relevant but I wanted to briefly explain how we think about Hooks.

Hooks are definitely "hiding" things. Or, depending on how you look at it, encapsulate them. In particular, they encapsulate local state and effects (I think our "effects" are the same things as "disposables"). The "implicitness" is in that they automatically attach the lifetime to the Component inside of which they're called.

This implicitness is not inherent in the model. You could imagine an argument being explicitly threaded through all calls — from the Component itself throughout custom Hooks, all the way to each primitive Hook. But in practice, we found that to be noisy and not actually useful. So we made currently executing Component implicit global state. This is similar to how throw in a VM searches upwards for the closest catch block instead of you passing around errorHandlerFrame in code.

Okay, so it's functions with implicit hidden state inside them, that seems bad? But in React, so are Components in general. That's the whole point of Components. They're functions that have a lifetime associated with them (which corresponds to a position in the UI tree). The reason Components themselves are not a footgun with regards to state is that you don't just call them from random code. You call them from other Components. So their lifetime makes sense because you remain in the context of UI code.

However, not all problems are component-shaped. Components combine two abilities: state+effects, and a lifetime tied to tree position. But we've found that the first ability is useful on its own. Just like functions are useful in general because they let you encapsulate code, we were lacking a primitive that would let us encapsulate (and reuse) state+effects bundles without necessarily creating a new node in the tree. That's what Hooks are. Components = Hooks + returned UI.

As I mentioned, an arbitrary function hiding contextual state is scary. This is why we enforce a convention via a linter. Hooks have "color" — if you use a Hook, your function is also a Hook. And the linter enforces that only Components or other Hooks may use Hooks. This removes the problem of arbitrary functions hiding contextual UI state because now they're no more implicit than Components themselves.

Conceptually, we don't view Hook calls as plain function calls. Like useState() is more use State() if we had the syntax. It would be a language feature. You can model something like Hooks with Algebraic Effects in languages that have effect tracking. So in that sense, they would be regular functions, but the fact that they "use" State would be a part of their type signature. Then you can think of React itself as a "handler" for this effect. Anyway, this is very theoretical but I wanted to point at prior art in terms of the programming model.

In practical terms, there are a few things here. First, it's worth noting Hooks aren't an "extra" API to React. They're the React API for writing Components at this point. I think I'd agree that as an extra feature they wouldn't be very compelling. So I don't know if they really make sense for Flutter which has an arguably different overall paradigm.

As for what they allow, I think the key feature is the ability to encapsulate state+effectful logic, and then chain it together like you would with regular function composition. Because the primitives are designed to compose, you can take some Hook output like useState(), pass it as an input to a cusom useGesture(state), then pass that as an input to several custom useSpring(gesture) calls which give you staggered values, and so on. Each of those pieces is completely unaware of the others and may be written by different people but they compose well together because state and effects are encapsulated and get "attached" to the enclosing Component. Here's a small demo of something like this, and an article where I briefly recap what Hooks are.

I want to emphasize this is not about reducing the boilerplate but about the ability to dynamically compose pipelines of stateful encapsulated logic. Note that it is fully reactive — i.e. it doesn't run once, but it reacts to all changes in properties over time. One way to think of them is they're like plugins in an audio signal pipeline. While I totally get the wary-ness about "functions that have memories" in practice we haven't found that to be a problem because they're completely isolated. In fact, that isolation is their primary feature. It would fall apart otherwise. So any codependence has to be expressed explicitly by returning and passing values into the next thing in the chain. And the fact that any custom Hook can add or remove state or effects without breaking (or even affecting) its consumers is another important feature from the third-party library perspective.

I don't know if this was helpful at all, but hope it sheds some perspective on the programming model. Happy to answer other questions.

rrousselGit commented 3 years ago

I'm definitely not suggesting using the builder approach, as the OP mentions, that has all kinds of problems. What I would suggest is just using initState/dispose. I don't really understand why that's a problem.

I'm curious how people feel about the code in #51752 (comment). I don't think it's any better than initState/dispose, but if people like hooks, do they like that too? Is hooks better? Worse?

The late keyword makes things better, but it still suffers from some issues:

Such Property may be useful for states that are self-contained or that do not depend on parameters that can change over time. But it may get difficult to use when in a different situation. More precisely, it lacks the "update" part.

For example, with StreamBuilder the stream listened can change over time. But there is no easy solution to implement such thing here, as the object is initialized only once.

Similarly, hooks have an equivalent to Widget's Key – which can cause a piece of state to be destroyed and re-created when that key changes.

An example of that is useMemo, which is a hook that cache an instance of object. Combined with keys, we can use useMemo to have implicit data fetching. For example, our widget may receive a message ID – which we then use to fetch the message details. But that message ID may change over time, so we may need to re-fetch the details.

With useMemo, this may look like:

String messageId;

Widget build(context) {
  final Future<Message> message = useMemo(() => fetchMessage(messageId), [messageId]);

}

In this situation, even if the build method is called again 10 times, as long as messageId does not change, the data-fetching is not performed again. But when the messageId changes, a new Future is created.


It's worth noting that I do not think flutter_hooks in its current state is refined for Dart. My implementation is more of a POC than a fully-fledged architecture. But I do believe that we have an issue with code reusability of StatefulWidgets.

I didn't remember where, but I remember suggesting that hooks in the ideal world would be a custom function generator, next to async* & sync*, which may be similar to what Dan suggest with use State rather than useState

Hixie commented 3 years ago

@gaearon

I want to emphasize this is not about reducing the boilerplate but about the ability to dynamically compose pipelines of stateful encapsulated logic.

That isn't the problem being discussed here. I recommend filing a separate bug to talk about the inability to do what you describe. (That sounds like a very different problem and honestly a more compelling one than the one described here.) This bug is specifically about how some of the logic is too verbose.

rrousselGit commented 3 years ago

No he is right, it is my wording that may be confusing. As I mentioned previously, this is not about the number of lines of code, but the lines of code themselves.

This is about factorizing state.

Hixie commented 3 years ago

This bug is extremely clear about the problem being "Reusing state logic is too verbose/difficult" and being all about how there is too much code in a State when you have a property that needs to have code to declare it, in initState, in dispose, and in debugFillProperties. If the problem you care about is something different then I recommend filing a new bug that describes that problem.

I really, really strongly recommend forgetting about hooks (or any solution) until you fully understand the problem you want to solve. It's only by having a clear understanding of the problem that you will be able to articulate a convincing argument in favour of a new feature, because we must evaluate features against the problems that they solve.

rrousselGit commented 3 years ago

I think you are misunderstanding what I said in that issue then.

The problem is by no mean boilerplate, but reusability.

Boilerplate is a consequence of an issue with reusability, not the cause

rrousselGit commented 3 years ago

What this issue describes is:

We may want to reuse/compose state logic. But the options available are either mixins, Builders, or not reusing it – all of which have their own issues.

The issues of the existing options may be related to boilerplate, but the problem we are trying to solve isn't. While reducing the boilerplate of Builders is one path (which is what hooks do), there may be a different one.

For example, something I wanted to suggest for a while was to add methods like:

context.onDidChangeDependencies(() {

});
context.onDispose(() {

});

But these have their own issues and do not fully solve the problem, so I didn't.

timsneath commented 3 years ago

@rrousselGit, feel free to edit the original problem statement at the top here to better reflect the problem. Also feel free to create a design doc: https://flutter.dev/docs/resources/design-docs that we can iterate on together (again, as @Hixie suggests, focusing for now on the tightest possible exposition of the problem statement). I'd love you to feel as empowered as any other Flutter engineer -- you're part of the team, so let's iterate together!

rrousselGit commented 3 years ago

I've looked through the issue again a few times. In all honesty, I do not understand where the misunderstanding is coming from, so I'm not sure what to improve. The original comment repeatedly mentions the desire for reusability/factorization. The mentions about boilerplate are not "Flutter is verbose" but "Some logics are not reusable"

I don't think the design doc suggestion is fair. It takes a significant amount of time to write such a document, and I am doing this in my free time. I am personally satisfied with hooks. I'm not authoring these issues in my interest, but to raise awareness about a problem that impacts a significant number of people.

A few weeks ago, I was hired to discuss architecture about an existing Flutter app. Their probably was exactly what is mentioned here:

Hixie commented 3 years ago
  • marking "messages" as read when some widgets become visible

That's an interesting case because it's similar to issues I've had in one of my own apps, so I looked at how I'd implemented the code there and I really don't see much of the problems that this bug describes, which may be why I'm having trouble understanding the problem. This is the code in question:

https://github.com/jocosocial/rainbowmonkey/blob/master/lib/src/views/forums.dart

Do you have examples of actual apps I could study to see the problem in action?

Hixie commented 3 years ago

(BTW, in general I would strongly recommend not using Streams at all. I think they generally make things worse.)

rrousselGit commented 3 years ago

(BTW, in general I would strongly recommend not using Streams at all. I think they generally make things worse.)

(I wholeheartedly agree. But the community currently has the opposite reaction. Maybe extracting ChangeNotifier/Listenable/ValueNotifier out of Flutter into an official package would help)

Do you have examples of actual apps I could study to see the problem in action?

Sadly no. I can only share the experience I had while helping others. I don't have an app at hand.

That's an interesting case because it's similar to issues I've had in one of my own apps, so I looked at how I'd implemented the code there and I really don't see much of the problems that this bug describes, which may be why I'm having trouble understanding the problem. This is the code in question:

In your implementation, the logic isn't tied to any life-cycle and placed inside build, so it kind of works around the problem. It may make sense in that specific case. I'm not sure if that example was good.

A better example may be pull-to-refresh.

In a typical pull-to-refresh, we will want:

And we'll want to implement such a feature for all resources and multiple screens. Furthermore, some screens may want to refresh multiple resources at once.

ChangeNotifier + provider + StatefulWidget will have quite a lot of difficulties factorizing this logic.

Whereas my latest experiments (which is immutability based & relies on flutter_hooks) supports the entire spectrum out of the box:

final productsProvider = FutureProvider<List<Product>>.autoDispose((ref) async {
  final cancelToken = CancelToken();
  ref.onDispose(cancelToken.cancel);

  return await repository.fetchProducts(cancelToken: cancelToken);
});

// ...

Widget build(context) {
  // Listens to the Future created by productsProvider and handles all the refresh logic
  AsyncValue<List<Product>> products = useRefreshProvider(
    productsProvider,
    // TODO consider making a custom hook to encaplusate the snackbar logic
    onErrorAfterRefresh: (err, stack) => Scaffold.of(context).showSnackBar(...),
  );

  return RefreshIndicator(
    onRefresh: () => context.refresh(productsProvider),
    child: products.when(
      loading: () {
        return const SingleChildScrollView(
          physics: AlwaysScrollableScrollPhysics(),
          child: CircularProgressIndicator(),
        );
      },
      error: (err, stack) {
        return SingleChildScrollView(
          physics: const AlwaysScrollableScrollPhysics(),
          child: Text('Oops, something unexpected happened\n$err'),
        );
      },
      data: (products) {
        return ListView.builder(
          itemCount: products.length,
          itemBuilder: (context, index) {
            return ProductItem(products[index]);
          },
        );
      },
    ),
  );
}

This logic is entirely self-contained. It can be reused with any resource inside any screens.

And if one screen wants to refresh multiple resources at once, we can do:

AsyncValue<First> first = useRefreshProvider(
  firstProvider,
  onErrorAfterRefresh: ...
);
AsyncValue<Second> second = useRefreshProvider(
  secondProvider,
  onErrorAfterRefresh: ...
);

return RefreshIndicator(
  onRefresh: () {
     return Future.wait([context.refesh(firstProvider), context.refresh(secondProvider)]);
  }
  ...
)
Hixie commented 3 years ago

I would recommend putting all of that logic in the app state, outside of the widgets, and just having the app state reflect the current app state. Pull to refresh needs no state within the widget, it just has to tell the ambient state that a refresh is pending and then wait for its future to complete.

rrousselGit commented 3 years ago

It isn't the responsibility of the ambient state to determine how to render an error vs loading vs data

Having this logic in the ambient state doesn't remove all logics from the UI The UI still need to determine whether to show the error in full screen or in a snack-bar It still need to force errors to be refreshed when the page is reloaded

And this is less reusable. If the rendering logic is fully defined in the widgets rather than the ambient state, then it will work with any Future and can even be included directly inside Flutter.

Hixie commented 3 years ago

I don't really understand what you are advocating for in your last comment. My point is that you don't need changes to the framework to do something just as simple as the refresh indicator code above, as is demonstrated by the code I cited earlier.

satvikpendem commented 3 years ago

If we have a lot of these types of interactions, not just for refresh indicators, but for animations, and others, it is better to encapsulate them where they are closest to being needed rather than putting them in the app state, because the app state doesn't need to know the specifics of every single interaction in the app if it's not needed in multiple places in the app.

rrousselGit commented 3 years ago

I don't think we are agreeing on the complexity of the feature and its reusability. Do you have an example which showcase that such feature is easy?

Hixie commented 3 years ago

I linked to the source of one app I wrote above. It's certainly not perfect code, and I plan to rewrite bits of it for the next release, but I didn't experience the problems you describe in this issue.

rrousselGit commented 3 years ago

But you are one of the tech leads of Flutter. Even when faced with a problem, you would have enough skill to immediately come up with a solution.

Yet on the other side, a significant number of people do not understand what is wrong with the following code:

FutureBuilder<User>(
  future: fetchUser(),
  builder: ...,
)

This fact is proven by how popular a Q/A I made on StackOverflow is.

The problem is not that it is impossible to abstract state logic in a reusable and robust way (otherwise there is no point in making this issue). The problem is that it requires both time and experience to do so.

By providing an official solution, this reduces the likelihood that an application ends up unmaintainable – which increases the overall productivity and developer experience. Not everyone could come up with your Property suggestion. If such a thing was built inside Flutter, it would be documented, get visibility, and ultimately help people that would have never thought about it to begin with.

Hixie commented 3 years ago

The problem is that it really depends on what your app is, what your state looks like, and so on. If the question here is just "how do you manage app state" then the answer isn't anything like hooks, it's lots of documentation talking about different ways to do it and recommending different techniques for different situations... basically, this set of documents: https://flutter.dev/docs/development/data-and-backend/state-mgmt

satvikpendem commented 3 years ago

There is ephemeral and app state, but there seems to be another use case as well: state that is only concerned with a single type of widget but that you nevertheless want to share between that type of widget.

For example, a ScrollController may invoke some type of animation, but it's not necessarily appropriate to put that into the global app state, because it's not data that needs to be used across all of the app. However, multiple ScrollControllers might have the same logic, and you want to share that life cycle logic between each of them. The state is still for only ScrollControllers, so not global app state, but copy-pasting the logic is prone to error.

Moreover, you may want to package this logic to make it more composable for your future projects, but also to others. If you look at the site useHooks, you'll see many pieces of logic that compartmentalize certain common actions. If you use useAuth you write it once and never have to worry about whether you missed an initState or dispose call, or whether the async function is has a then and catch. The function is written only once so the room for error basically disappears. Therefore, this kind of solution is not only more composable for multiple parts of the same app and between multiple apps, but it is also safer for the end programmer.

Hixie commented 3 years ago

I have no objection to people using hooks. As far as I can tell, nothing is preventing that. (If something is preventing that, then please file a bug about that.)

This bug isn't about hooks, it's about "Reusing state logic is too verbose/difficult", and I'm still struggling to understand why this requires any changes to Flutter. There's been many examples (including hooks) showing how it's possible to avoid the verbosity by structuring one's application one way or another, and there's lots of documentation about it already.

satvikpendem commented 3 years ago

I see, so you're asking why, if there exists something like a hooks package which was built with no changes to Flutter already, there needs to be a first party solution for hooks? I suppose @rrousselGit can answer this better but the answer probably involves better support, more integrated support and more people using them.

I can agree with you that besides that, I am also confused why any fundamental changes need to be made to Flutter to support hooks, since ostensibly the flutter_hooks package exists already.

rrousselGit commented 3 years ago

I'm still struggling to understand why this requires any changes to Flutter.

Saying that this problem is solved because the community made a package is like saying that Dart doesn't need data-classes + union types because I made Freezed. Freezed may be quite liked by the community as a solution to both of these problems, but we still can do better.

The Flutter team has a lot more leverage than the community ever will. You have both the capability to modify the entire stack; people that are experts on each individual part; and a salary to sponsor the work needed.

This problem needs that. Remember: One of the goals of the React team is for hooks to be part of the language, kind of like with JSX.

Even without language support, we still need work in analyzer; dartpad; flutter/devtools; and many hooks to simplify all the different things that Flutter does (such as for implicit animations, forms, and more).

timsneath commented 3 years ago

That's a good argument, I agree, even though the general philosophy of Flutter to have a small core. For that reason, we've been increasingly adding new functionality as packages even when it comes from Google, c.f. characters and animations. That gives us greater flexibility to learn and change over time. We would do the same for this space, unless there's a compelling technical reason why a package was insufficient (and with extension methods, that's even less likely than ever).

Putting things into the core of Flutter is tricky. One challenge is, as you know well from first-hand experience, is that state is an area that is evolving as we all learn more about what works well in a reactive UI architecture. Two years ago, if we'd been forced to pick a winner, we might have selected BLoC, but then of course your provider package took over and is now our default recommendation.

I could comfortably conceive of Google-employed contributors supporting flutter_hooks or a similar hooks package that had traction (although we have plenty of other work that is competing for our attention, obviously). In particular, we should If you're looking for us to take it over from you, that's obviously a different question.

satvikpendem commented 3 years ago

Interesting argument, @timsneath. The Rust community also does something similar, because once introduced into the core or standard library of a language or framework, it is very difficult to take it out. In Rust's case, it is impossible as they want to maintain backwards compatibility forever. Therefore, they wait until packages have arrived and competed with each other until only a few winners emerge, then they fold that into the language.

This could be a similar case with Flutter. There might be something better than hooks later on, just as React had to move from classes to hooks but still had to maintain classes, and people had to migrate. It might then be better to have competing state management solutions before being added to the core. And perhaps we the community should innovate on top of hooks or try finding even better solutions.

rrousselGit commented 3 years ago

I understand that concern, but this isn't about a state management solution.

Such feature is closer to Inheritedwidget & StatefulWidget. It is a low level primitive, that could be as low as a language feature.

Hooks may be independent from the framework, but that's only through luck. As I mentioned before, another path to this problem may be:

context.onDispose(() {

});

And similar event listeners. But that is impossible to implement out of the framework.

I do not know what the team would come up with. But we can't exclude the possibility that such solution would have to be directly next to Element

Hixie commented 3 years ago

Do extensions help with that?