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 for hooks #24474

Closed
rrousselGit opened this issue Nov 17, 2018 · 38 comments
Closed

Proposal for hooks #24474

rrousselGit opened this issue Nov 17, 2018 · 38 comments
Labels
c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels.

Comments

@rrousselGit
Copy link
Contributor

rrousselGit commented Nov 17, 2018

React team recently announced hooks: https://medium.com/@dan_abramov/making-sense-of-react-hooks-fdbde8803889

Due to how similar Flutter is with React, it would probably be interesting to see if these fits to Flutter too.

Hooks are basically widgets within widgets, but without build method and an Element. They will listen to all the creator widget's life-cycles and are able to store mutable data (Even within a StatelessWidget).

Hooks are typically used to improve code-sharing between widgets and readability by regrouping all the implementation of a specific feature into one shareable place.


It is relatively easy to implements hooks in Flutter. Everything can be done within Element and by exposing a new method on BuildContext.
What I had in mind was adding the following methods to BuildContext:

abstract class BuildContext {
  // Hook is similar to Widget but without an Element
  HookState<T> useHook<T extends Hook>(T hook);
  AsyncSnapshot<T> useStream<T>(Stream<T> stream, { T initialData } );
  ValueNotifier<T> useState<T>([T defaultValue]);
  void useListenable(Listenable listenable);
  T useValueListenable(ValueListenable<T> valueListenable);
}

The interesting part here is that it potentially makes the boilerplate associated with Stream and ValueListenable much smaller:

The usual

StreamBuilder<T>(
  stream: Bloc.of(context).stream,
  builder: (context, snapshot) {
    // do something
  }
);

Becomes a one-liner:

final snapshot = context.useStream(Bloc.of(context).stream));
// do something

This can be very interesting for BLoC pattern, which is quite popular.


As pointed out by @miyoyo, supermixins potentially replace the needs for hooks.
They are one not a total replacement for them though, so it's still open to discussion.

One thing hooks do that supermixins don't is being able to expose public state. Mixins cannot do that since it would lead to interface conflict.

It would be possible to extract that logic into a widget, but it'd require many callbacks to keep these synchronized. With hooks this isn't needed anymore

EDIT:

A few more important points have been added on a later comment: #24474 (comment)

@rrousselGit
Copy link
Contributor Author

I made a proposal implementation here: #24477

This needs tests/documentation, but overall it works

@miyoyo
Copy link

miyoyo commented Nov 18, 2018

This implementation looks flawed in the context of BLoC, as there is no way to specifically define which part of the interface the hook will affect.

This will lead to increased complexity to automagically figure out what part of the tree needs rebuilding, or a way to link the snapshots with a part of the tree, which either means you've effectively recreated streambuilders, but added coupling, or you've recreated setState, with added coupling too.

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Nov 18, 2018

This implementation looks flawed in the context of BLoC, as there is no way to specifically define which part of the interface the hook will affect.

There is: what's rebuilt is the BuildContext used.

You can use a different if you want to:

Widget build(BuildContext context) {
  return Center(
    child: Builder(
      builder: (context) {
        final bloc = Bloc.of(context);
        return Text(context.useStream(bloc.stream).data ?? "Foo");
      }, 
  );
}

Hooks are not limited to bloc. They are a solution to reorganize your implementation by regrouping everything related into one place.

@miyoyo
Copy link

miyoyo commented Nov 18, 2018

Even then, you haven't provided an answer that's better than what the current StreamBuilders are, you just moved the streams to the context and swapped the StreamBuilder for a Builder.

BLoCs already allow you to reorganize your implementation by regrouping everything related into one place. Scoped_Model too, technically.

@rrousselGit
Copy link
Contributor Author

I think you misunderstand the purpose of hooks: Hooks are not related to BLoC or Streams.

The useStream is just one of the many potential usages of hooks.

The article on the top-most comment explains a bit more about hooks. It's in react, but it will still probably be better than if I explained it myself.

@miyoyo
Copy link

miyoyo commented Nov 18, 2018

The reason I'm comparing them to BLoC is that they're effectively very similar, in react they're used to listen to a value and change components accordingly, which is essentially what BLoC does using streams.

Or InheritedWidget/InheritedModel/Scoped_Model with notifyListeners()

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Nov 18, 2018

The reason I'm comparing them to BLoC is that they're effectively very similar

Not really. Hooks are like advanced mixins, where multiple hooks can implements the same method.
Let's say you wanted to log the duration a widget lived.

With current widgets you'd typically have that:

class Bar extends StatefulWidget {
  @override
  _BarState createState() => _BarState();
}

class _BarState extends State<Bar> {
  DateTime start = DateTime.now();

  @override
  void dispose() {
    print(DateTime.now().compareTo(start));
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    // something
  }
}

But this logic is impossible to extract, it is deeply linked with Bar widget. So you basically have to recode it again everytime you want it on another widget.

With hooks, that's a whole new story. You can extract that logic into a custom hook as followed:

class MyHook extends Hook {
  @override
  _MyHookState createState() => _MyHookState();
}

class _MyHookState extends HookState<MyHook> {
  DateTime start = DateTime.now();

  @override
  void dispose() {
    print(DateTime.now().compareTo(start));
    super.dispose();
  }
}

and then reuse it into any widgets like so:

class Foo extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    context.useHook(MyHook());
    // do something
  }
}

@miyoyo
Copy link

miyoyo commented Nov 18, 2018

But this logic is impossible to extract.

I can do that with mixins

class MyWidget extends StatefulWidget {
  @override
  _MyWidgetState createState() => _MyWidgetState();
}

class _MyWidgetState extends State<MyWidget> with LogOnDispose {
  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

mixin LogOnDispose<T extends StatefulWidget> on State<T> {
  DateTime start = DateTime.now();
  @override
  void dispose() {
    print(DateTime.now().difference(start));
    super.dispose();
  }
}

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Nov 18, 2018

No, like I said hooks are advanced mixins.

It is impossible to have multiple mixins implementing the same method:

mixin Bar on State {
  int value = 42;
  dispose() {
    print(value.toString());
  }
}

mixin Foo on State {
  String value = "42";
  dispose() {
    print(value);
  }
}

class MyWidget extends StatefulWidget {
  @override
  _MyWidgetState createState() => _MyWidgetState();
}


class _MyWidgetState extends State<MyWidget> with Foo, Bar {

}

With hooks, this works

@miyoyo
Copy link

miyoyo commented Nov 18, 2018

Once again, I can do that with supermixins

class MyWidget extends StatefulWidget {
  @override
  _MyWidgetState createState() => _MyWidgetState();
}

class _MyWidgetState extends State<MyWidget> with First, Second, Third {
  @override
  Widget build(BuildContext context) {
    return MaterialButton(
      child: Text("dispose"),
      onPressed: dispose,
    );
  }
}

mixin First<T extends StatefulWidget> on State<T> {
  @override
  void dispose() {
    print("first");
    super.dispose();
  }
}

mixin Second<T extends StatefulWidget> on State<T> {
  @override
  void dispose() {
    print("second");
    super.dispose();
  }
}

mixin Third<T extends StatefulWidget> on State<T> {
  @override
  void dispose() {
    print("third");
    super.dispose();
  }
}

This prints

I/flutter (28072): third
I/flutter (28072): second
I/flutter (28072): first

@rrousselGit
Copy link
Contributor Author

Nice. I didn't know dart mixins could do that.
But what about the compilation error from my previous example due to prototype conflict?

Superinterfaces don't have a valid override for 'value': Foo.value (() → String), Bar.value (() → int). [inconsistent_inheritance]

Although I'd agree that supermixins reduce quite a lot the need for hooks.

@miyoyo
Copy link

miyoyo commented Nov 18, 2018

This error happens when you have the same name for a value, but different types, in this case, it seems like you have one method returning an int and another returning a string (maybe it's a getter that generates the error?)

@rrousselGit
Copy link
Contributor Author

This was purposefully done to show that hooks are independent and therefore cannot have conflicts but mixins do.

An example of hooks React gave is extracting the input logic. We could potentially have the same in Flutter:

class Foo extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final name = context.useHook(InputHook());
    final surname = context.useHook(InputHook());
    
    return Column(
      children: <Widget>[
        TextField(controller: name.controller),
        TextField(controller: surname.controller),
      ],
    );
  }
}

(Not that it's necessarily wanted to do that in Flutter)

I'll join your side into saying that supermixins are likely a better solution. But they don't cover all the potential use-cases, so I'll leave this open for discussion. Maybe there's a use-case I didn't think of.

@rrousselGit
Copy link
Contributor Author

From my understanding of React hooks, they are used as a complete replacement to our current StatefulWidget not just here to hook life-cycles. Which mixins can't do.

One thing for sure is that it heavily reduces the number of lines required to make "Components" and makes them more organized; Which is one of the biggest downside Flutter has for its widgets at the moment.

But I'm not sure wether their API entirely fits to Flutter

@miyoyo
Copy link

miyoyo commented Nov 19, 2018

Would you be able to provide a mockup of what these theoretical "HookWidgets" might look like? I honestly can't seem to find a use case that you can't just use a class, mixin or stream to simulate 🤔

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Nov 19, 2018

It's not that we can't do it with classes. It's a different kind of architecture for your components.
React team said any new components should be done using hooks instead of the usual class.

Here's a gif showing visually how it changes the architecture of React components:

ezgif com-crop

In flutter a typical comparison would be with RouteAware

Currently we have this:

class Foo extends StatefulWidget {
  @override
  _FooState createState() => _FooState();
}

class _FooState extends State<Foo> with RouteAware {
  bool visible;

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    routeObserver.subscribe(this, ModalRoute.of(context));
  }

  @override
  void dispose() {
    routeObserver.unsubscribe(this);
    super.dispose();
  }

  @override
  void didPushNext() {
    setState(() {
      visible = false;
    });
    super.didPushNext();
  }

  @override
  void didPopNext() {
    setState(() {
      visible = true;
    });
  }

  @override
  Widget build(BuildContext context) {
    return Text("$visible");
  }
}

That's tons of lines for not much and it's hardly shareable into a mixin because of the visible field.

We could potentially make a widget for it, but we'll end up in the callback hell.

With hooks we have this instead:

class Foo extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final visible = useRouteAware(context, routeObserver);
    return Text('$visible');
  }
}

Where the hook is independent from Foo and is :

bool useRouteAware(BuildContext context, RouteObserver routeObserver) {
  final __HookState state = context.useHook(_Hook(routeObserver));
  return state.visible;
}

class _Hook extends Hook {
  final RouteObserver routeObserver;

  _Hook (this.routeObserver);

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

class __HookState extends HookState<_Hook k> {
  bool visible;

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    routeObserver.subscribe(this, ModalRoute.of(context));
  }

  @override
  void dispose() {
    routeObserver.unsubscribe(this);
    super.dispose();
  }

  @override
  void didPushNext() {
    setState(() {
      visible = false;
    });
    super.didPushNext();
  }

  @override
  void didPopNext() {
    setState(() {
      visible = true;
    });
  }
}

@miyoyo
Copy link

miyoyo commented Nov 19, 2018

Finally, a good example!

Tried to make it with mixins, but I don't have access to a device yet. What do you think about this?

class App extends StatefulWidget {
  @override
  AppState createState() {
    return new AppState();
  }
}

class AppState extends State<App> {
  var _observers = Set<NavigatorObserver>();
  
  @override
  Widget build(BuildContext context) {
    return new MaterialApp(
      title: 'Flutter Demo',
      theme: new ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: new MyHomePage(title: 'Flutter Demo Home Page'),
      navigatorObservers: _observers.toList(),
    );
  }

  static void addObserver(BuildContext context, NavigatorObserver observer){
    final root = context.ancestorStateOfType(TypeMatcher<AppState>()) as AppState; //Maybe TypeMatcher<App>() ?
    root.setState((){
      root._observers.add(observer);
    });
  }
}

//...

class ObserveTheRoute extends StatefulWidget {
  @override
  _ObserveTheRouteState createState() => _ObserveTheRouteState();
}

class _ObserveTheRouteState extends State<ObserveTheRoute> with RouteAwareWidget {

  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

mixin RouteAwareWidget<T extends StatefulWidget> on State<T> implements RouteAware {
  final _observer = RouteObserver();

  var visible = true;

  @override
  void initState(){
    scheduleMicrotask((){
      Root.addObserver(context, _observer);
      _observer.subscribe(this, ModalRoute.of(context));
    });
    super.initState();
  }

  @override
  void dispose() {
    _observer.unsubscribe(this);
    super.dispose();
  }

  void didPushNext() {
    setState(() {
      visible = false;
    });
  }

  void didPopNext() {
    setState(() {
      visible = true;
    });
  }

  void didPop() {}
  void didPush() {}
}

It should work in theory, not sure about the typematcher in the root widget.

Clear downside here: the RouteAware interface must be implemented, leading to having to copy/paste the implementation of RouteAware (Empty in this case but you know, examples).

For readonly access to the variable, you could use a private + getter system

  var _visible = true; //Replace the other visible by _visible
  get visible => _visible;

@rrousselGit
Copy link
Contributor Author

Your example should be fine overall. The biggest issue here is that on a bigger widget, we may want to use multiple mixins to extract the logic.

But due to how mixins works, it's likely that we'd end up with conflicts on the public variables like visible. If two mixins tries to define a public variable under the same name but with different type, it'll break (as pointed out by my earlier example).

Since hooks are unrelated to the Widget itself, they don't suffer from this issue; which is the main benefit of hooks.

@miyoyo
Copy link

miyoyo commented Nov 19, 2018

How about a language proposal to allow for named mixins?
That should avoid conflicts, but might cause problems if used in an external context.
Thinking about it, another solution might be to use classes but pass ´this´ to the classes, letting them works similarly to hooks without having to re-pipe anything.

Or even just setState, thinking about it

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Nov 19, 2018

How about a language proposal to allow for named mixins?

and

Thinking about it, another solution might be to use classes but pass ´this´ to the classes,

Interesting ideas. Although I'm not too sure how they would be implemented. Do you have an example?

But both proposal miss one thing hooks do: We can use the same hook multiple times if we wanted to.
We can think of having the following (even if it's useless in this example):

@override
Widget build(BuildContext context) {
  final visible = useRouteAware(context, routeObserver);
  final visible2 = useRouteAware(context, routeObserver2);
  return Container();
}

@miyoyo
Copy link

miyoyo commented Nov 19, 2018

Nice! Tricky one, this is actually getting fun to solve 🤔

class ObserveTheRoute extends StatefulWidget {
  @override
  _ObserveTheRouteState createState() => _ObserveTheRouteState();
}

class _ObserveTheRouteState extends State<ObserveTheRoute> with DisposeQueue {
  @override
  Widget build(BuildContext context) {
    var a = RouteAwareVisible(this);
    return Text("$a");
  }
}

mixin DisposeQueue<T extends StatefulWidget> on State<T> {
  final _disposeQueue = <VoidCallback>[];

  void queueForDisposal(VoidCallback f) => _disposeQueue.add(f);

  @override
  void dispose() {
    _disposeQueue.forEach((f)=>f());
    super.dispose();
  }
}

class RouteAwareVisible extends RouteAware {
  final _observer = RouteObserver();
  final widget;

  RouteAwareVisible(this.widget){
    AppState.addObserver(widget.context, _observer);
    widget.queueForDisposal((){
      _observer.unsubscribe(this);
    });
  }

  var _visible = true;

  @override
  void didPushNext() {
    widget.setState(() {
      _visible = false;
    });
    super.didPushNext();
  }

  @override
  void didPopNext() {
    widget.setState(() {
      _visible = true;
    });
    super.didPopNext();
  }

  String toString() => "$_visible";
}

Interesting observation: You can't use type arguments with mixins:

class Something<T extends Some with Thing> {

I don't know if it's intended behavior or just undefined.

@zoechi zoechi added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. labels Nov 22, 2018
@rrousselGit
Copy link
Contributor Author

I've managed to make them working without editing Flutter sources, by introducing a new kind of widget.

This is currently in alpha here https://github.com/rrousselGit/flutter_hooks
I'll provide a more production-ready implementation soon.

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

I don't see where you explicitly call a dispose on the streams, do you have to call it manually?

You could try to piggyback off element.unmount() however this leaves no possibility of keeping state when changing pages or scrolling down a list.

@rrousselGit
Copy link
Contributor Author

Not implemented yet, just a proof of concept. But yeah it'd be an override of Element.unmount

however this leaves no possibility of keeping state when changing pages or scrolling down a list.

That's strictly the same story as with StreamBuilder or similar.

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

Fair, you'd still need to implement something like https://docs.flutter.io/flutter/widgets/AutomaticKeepAliveClientMixin-mixin.html to keep the state of hooks in lists, otherwise they'd be garbage collected.

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Dec 3, 2018

Debatable. I don't see any nice solution to this, as some hooks may want to be disposed and other don't.

I'd say it's much easier to simply wrap the widget into a KeepAlive and leave that logic to the widget, not the hook.

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

Sure, but what if you had a list of widgets that, in some cases needs to be kept alive, and in others, don't?

This is exactly what addAutomaticKeepAlive and AutomaticKeepAliveClientMixin are for

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

No. AutomaticKeepAliveClientMixin produces KeepAliveNotifications to indicate to an AutomaticKeepAlive that this widget is supposed to be kept alive in certain circumstances, this is not the same thing as just using a KeepAlive.

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Dec 3, 2018

Sorry, got confused. I was thinking of this:

class KeepAliveClient extends StatefulWidget {
  final bool keepAlive;
  final Widget child;

  const KeepAliveClient({Key key, @required this.keepAlive, this.child})
      : super(key: key);

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

class _KeepAliveClientState extends State<KeepAliveClient>
    with AutomaticKeepAliveClientMixin {
  @override
  Widget build(BuildContext context) {
    return widget.child;
  }

  @override
  bool get wantKeepAlive => widget.keepAlive;
}

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

How about using StatefulWidget internally instead of StatelessWidget?
This would bring you things like access to dispose, initState, etc. and keep compatibility with explicit animations, and mixins that are only valid for State objects.

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Dec 3, 2018

Can you elaborate what you mean by internally?

My goal with hooks is to combine it to functional widgets using https://github.com/rrousselGit/functional_widget. So there are no classes involved at all in the writing, so no place for mixins/methods:

@widget
Widget myWidget(HookContext context, { int parameter }) {
  final value = context.useStream(bloc.foo).requireData;
  return Text('$value $parameter');
}

This is a huge reduction of boilerplate

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

I take it it's impossible to do animations with this, as it's impossible to add anything via mixins.

Beyond that, why bother integrating hooks into flutter when you could just go the simpler route and use codegen everywhere?

Which would also allow for more efficient code, as you won't need to rebuild the entire widget when only one part of it needs to be notified of an update.

@rrousselGit
Copy link
Contributor Author

Things like SingleTickerProviderMixin can be extracted into a hook that returns a TickerProvider. Which can be combined with yet another hook to create an AnimationController.

Beyond that, why bother integrating hooks into flutter when you could just go the simpler route and use codegen everywhere?

Can you elaborate that too?

as you won't need to rebuild the entire widget when only one part of it needs to be notified of an update.

Nor do need hooks. You don't have to put all your hooks on the top level widget. You can insert them on any given level and achieve the desired behavior.

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

As it currently stands, your hook system just MarkNeedBuilds your element, which will rebuild itself and all of it's children, if someone has a widget like your example, if controller2 fires, both Texts are re-evaluated, but v1 didn't change.

@rrousselGit
Copy link
Contributor Author

rrousselGit commented Dec 3, 2018

As you pointed out it's linked to the Element.
This example showcase that you can use multiple times the same hook. But it doesn't force you to.

It's like with other widgets. You can split that logic across multiple Elements.

@miyoyo
Copy link

miyoyo commented Dec 3, 2018

Sure, but if you were to use code generation, you could automatically improve performance by encapsulating the state in either automatically generated sub-widgets, or a builder of some kind.

@rrousselGit
Copy link
Contributor Author

Closed in favor of #25280

I made a recap with a better explanation of their pro-cons.

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

No branches or pull requests

3 participants