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

Flutter's Listenable use case #23

Open
goderbauer opened this issue Jul 19, 2021 · 11 comments
Open

Flutter's Listenable use case #23

goderbauer opened this issue Jul 19, 2021 · 11 comments

Comments

@goderbauer
Copy link
Collaborator

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

  • listener is added in initState
  • listener is moved to new object in didUpdateWidget
  • listener is removed in dispose
  • (often) a method calling an empty setState is needed

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
Copy link
Collaborator Author

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 (#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
Copy link
Owner

jakemac53 commented Jul 20, 2021

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 #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
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

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:

flutter/flutter#51752
flutter/flutter#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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

goderbauer commented Jul 20, 2021

@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
Copy link
Contributor

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

jakemac53 added a commit that referenced this issue Aug 9, 2021
Towards #23

I want to follow up with exploring use cases for listenable objects that come from the build context next.

This also has some extra cleanup in the `fromParts` apis - I realized it doesn't make sense for them to auto-concat nested lists of code with a `,`. Instead you can supply a separator if you wish, otherwise pieces of code are just concatenated directly.

I also did remove the usage of `autoDispose` from the example - right now the prototype implementation doesn't support mixing these two macros together well, I filed a couple of related issues on that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants