jakemac53 / macro_prototype

A very basic prototype of macros using build_runner
BSD 3-Clause "New" or "Revised" License
73 stars 7 forks source link

Flutter's Listenable use case #23

Open goderbauer opened 3 years ago

goderbauer commented 3 years ago

Using Listenables in stateful widgets often has a lot of boilerplate, see example in https://gist.github.com/goderbauer/b82efcb154c3fe1814a0d0b60d277def:

We discussed this use case in our initial brainstorm and I am now curious if we could reduce this boilerplate using this macro prototype.

When I experimented with this, I often got stuck wanting to put a macro on the controller field in the StatefulWidget, which would then generate the required logic in the initState, didUpdateWidget, and dispose methods of the associated State class. However, having the macro on a field affect methods in another class isn't possible. :)

Opening this issue to discuss other ideas for how this boilerplate can be reduced with macros.

goderbauer commented 3 years ago

Another idea I had: Maybe we can put a macro on the State class. This macro would then walk the fields of the associated StatefulWidget and generate the initState, didUpdateWidget, and dispose implementations for all fields of type Listenable.

I believe in order to get the types of those fields, we need a ClassDefinitionMacro, which - while mentioned in the readme - doesn't appear to be implemented (https://github.com/jakemac53/macro_prototype/issues/22).

I'd imagine once we have that, we could do

(ClassDefinition.superclass.typeArguments.first as ClassDefinition).fields.forEach(
  (FieldDefinition f) { 
    // check if `f.type is Listenable`, add logic to initState, didUpdateWidget, and dispose for it, etc.
  }
);`

Maybe?

jakemac53 commented 3 years ago

I do think putting the macro on the state class is the best approach here - unless you just wanted to generate the entire state class but I think that is not actually feasible for real world cases.

Beyond the issue you were talking about, which I think is solvable (we just need to define the api), I am not sure how we would want to hook up the _textChanged method here. One idea is to actually annotate that method, with something that tells it which field from the parent widget the function is associated with. Unless that method should also be generated, and is expected to always be that simple?

Basically I am imagining something like this:

class _MyExampleState extends State<MyExample> with SingleTickerProviderStateMixin {
  @autoListen('widget.controller') // A bit weird/unfortunate to use a string here.
  void _textChanged() {
    setState(() {
      // The controller state changed.
    });
  }

  @override
  Widget build(BuildContext context) {
    // Doing something useful with it.
    return Text(widget.controller.text);
  }
}

This would be a MethodDeclarationMacro, as well as a MethodDefinitionMacro. In the declaration phase it would declare all the methods that it wants to generate (as external methods), with itself as an annotation. If the methods already exist it would need to add itself to those declarations, which would require https://github.com/jakemac53/macro_prototype/issues/8. Then in the definition phase it would either wrap the existing method with the extra code or it would implement the external methods it generated.

How does that sound?

rrousselGit commented 3 years ago

This is similar to the flutter_hooks / reactive state for widgets discussion, where similar proposals where made.

The problem is this approach is, it isn't composable and has a hard dependency on where the value is coming from.

We should not assume that the Listenable is a property on widget. It could be internal state defined in the State class too, or coming from InheritedWidgets/DI

jakemac53 commented 3 years ago

The problem is this approach is, it isn't composable and has a hard dependency on where the value is coming from.

We should not assume that the Listenable is a property on widget. It could be internal state defined in the State class too, or coming from InheritedWidgets/DI

This actually partly why I left it as a String - that is basically just any arbitrary expression that evaluates to an instance of ChangeNotifier. There is no such tight coupling in this scenario, it can come from anywhere.

jakemac53 commented 3 years ago

I see the didUpdateWidget part would get a bit interesting. You may end up wanting to distinguish specifically in the api between accessing things from the widget versus other places.

rrousselGit commented 3 years ago

This actually partly why I left it as a String - that is basically just any arbitrary expression that evaluates to an instance of ChangeNotifier. There is no such tight coupling in this scenario, it can come from anywhere.

Anything more complex than widget properties will likely be too much for the "String" to handle.

I don't think asking people to write things like:

@SomAnnotation('InheritedWidget.of(context).someListenable')
void onSomeListenableChange() {
  //
}

is a good idea. Code as String doesn't scale well.

I'm also curious on how you would deal with ordering. Because those String queries could have dependencies that are modified by other listeners.

Like:

class SomeState extends State<...> {
  // how many times widget.someValue was modified
  int someValueChangeCount = 0;

  @autoListen('widget.someValue')
  onSomeValueChange() {
    someValueChangeCount++;
  }

  @autoListen('someValueChangeCount > 10')
  onSomeValueChangeCount() {
    print('Wow, someValue changed a lot!');
  }
}

Because if you checked for changes to those queries only within widget life-cycles, chances are that "someValueChangeCount > 10" is tested before onSomeValueChange was able to execute.

jakemac53 commented 3 years ago

Anything more complex than widget properties will likely be too much for the "String" to handle.

These can always be extracted out into getters if they get complicated, but I generally agree a String isn't great.

@autoListen('someValueChangeCount > 10')

This feature afaik is only for ChangeNotifier properties, so this type of expression (with a comparison) wouldn't be valid in this context. I don't believe the ordering of change notifier subscriptions should matter? As far as I can tell those notifications are delivered synchronously, so its dependent on when the values change.

I think what you really want here is for @autoListen('someValueChangeCount > 10') to subscribe to someValueChangeCount, and then re-execute the expression and invoke the method if that evaluates to true. I don't see any ordering concerns here, as that would just be listening to changes of someValueChangeCount.

rrousselGit commented 3 years ago

This feature afaik is only for ChangeNotifier properties, so this type of expression

This issue is. But this issue is only a subset of a larger popular Flutter issue:

https://github.com/flutter/flutter/issues/51752 https://github.com/flutter/flutter/issues/25280

Which are not specific to Listenables/ChangeNotifier. Those Flutter issues encapsulate this, the autoDispose, the functional widget and more.

Those issues would want to implement the "someValueChangeCount > 10" queries in a way that is not bound to ChangeNotifier.

goderbauer commented 3 years ago

I am not sure how we would want to hook up the _textChanged method here.

I was wondering this one as well. In some real world use cases you probably need to do more than just calling setState, so some kind of customization hook to the macro will be needed. Ideally, those references wouldn't be passed in as strings, though.

Related to this, when I was playing with the prototype I was also wondering if there could be a way to pass a function to a macro, which can then be called from the generated code. Something like that would allow us to do something like:

@autoListener((MyWidget widget) => widget.controller)
void _textChanged() {
  // Do something.
}
goderbauer commented 3 years ago

@rrousselGit For the someValueChangeCount > 10 case wouldn't something like this be pretty clear and concise:

@autoListener('widget.controller') // Well, hopefully something that doesn't use strings...
void _textChanged() {
  if (widget.controller.value > 10) {
   print('Wow, someValue changed a lot!');
  }
}

I would imagine that in many cases the condition > 10 may also not be available at compile time since it may depends on other data...

rrousselGit commented 3 years ago

What you described isn't quite matching what I described. But I am probably highjacking the issue to talk about a broader topic. I will create a new more focused issue instead