letsar / binder

A lightweight, yet powerful way to bind your application state with your business logic.
MIT License
178 stars 12 forks source link

Few questions regarding best practices using the "binder" library #2

Closed Delgan closed 3 years ago

Delgan commented 4 years ago

Hi!

Let's get straight to the point: I fell in love with your library. The architecture separating view/logic/state that you set up is what I have always aimed for but never managed to achieve. You succeeded brilliantly in establishing a simple but complete design.

Before that I've done countless refactors of my application. I've tried different approaches without ever finding one that fully satisfied me. Finally, I stumbled upon your creation last week by coincidence. I decided to give it a try for yet another refactor... It has been an incredible pleasure. This solved all the difficulties, trade-offs and accidental complexities I had been facing until then. My code is so much more modular, it's a breath of fresh air. My views are much less nested. It is infinitely simpler to share the same state between several business and UI components. I love how simple and elegant it is, and I love how development just has to let itself be guided by your api.

In short: state management in Flutter is a solved problem thanks to binder.


While I was re-writing my application, I noted a few points of interest that I would have liked to clarify. I hope you don't mind if I ask you about some of these. A short answer will suffice, I don't want to abuse your time. It's simply that I want to make the best use of your library.

Is there any purpose for the public scope attribute of a Logic? Each class inheriting Logic has a public Scope. I understand what it stands for, but is there any good reason to directly access this object instead of calling use()/read()?

What happens if I initialize a reference inside a widget? All the examples show the use of LogicRef and StateRef being global variables. I was wondering: is it a problem if one is initialized as a widget attribute? There is no risk of memory leak or callback called several times?

Is there any reason why it is not possible to call context.read() for a Computed? In particular, if one wants to access the value in a callback. It's possible to use context.read() on a StateRef but not on a Computed (which I thought would be similar to a StateRef as it is derived from). Why that?

What is the best way to request data while building a widget? Sometimes, my View or Widget does not depend on an actual state but rather on temporal data. For example, when I need to display a list of values loaded from the cloud. How do you usually load and store this ephemeral data from a LogicRef? I do something similar to this:

FutureBuilder(
    future: Future.microtask(() => context.use(myLogicRef).loadData()),
    builder: (context, snapshot) => ...,
)

Is this an acceptable workaround, or does it indicate a problem in the structure of my application?

How to handle the lifetime of one or more Logic "instances"? So, let's say inside my application there is a chess game. I implemented it using one ChessGameLogic which involves many states: chessBoardRef, blunderCountRef, turnRef, elapsedTimeRef, isGameFinishedRef, winnerRef, etc. So anytime the user navigates to the /chess page a new game must be started. By starting a new game, all previously listed states need to be reset to their initial value. When the game ends and is not used by any view, the refs can be disposed. What would be the best way to use binder in this case?

I could declare an init() function in my logic and make my view call it before starting the game. However, if my user is very strong, I want to display two games at the same time for him to play them simultaneously. That means I need two separate sets of states. Is it a good use case for overrideWith()?

I was thinking of declaring a function inside the logic file like that (note that I'm overriding the logic reference too):

List<BinderOverride> createGameInstance() => [
    chessGameLogicRef.overrideWith((scope) => ChessGameLogic(scope)),
    chessBoardRef.overrideWith(ChessBoard()),
    blunderCountRef.overrideWith(0),
    ...
]

And declare a new BinderScope in the view file:

BinderScope(
    overrides: createGameInstance(),
    child: GameChessView(),
)

Would you say that this is the idiomatic way to handle the Logic as if it were a class instance? I mean, the logic and states do not actually need to exist by default. They can be instantiated independently one or multiple times, and they should "die" when the view no longer use it.

Well, that's it for tonight. :) Anyway, thank you very much for the work you did!

letsar commented 4 years ago

Whoa, this is so great to read this kind of feedback πŸ’™ ! Thank you so much! It's so grateful to see that the work I have been doing is useful for you!

I will try to answer to you the best I can πŸ™‚ :

Is there any purpose for the public scope attribute of a Logic? Each class inheriting Logic has a public Scope. I understand what it stands for, but is there any good reason to directly access this object instead of calling use()/read()?

This is a technical reason. The scope attribute is needed internally by the read/write/use methods. I didn't want business logics to directly inherit from a type coming from binder. Otherwise I would have find binder too much intrusive. Instead I decided to go with a mixin, which allows the user to call the utility methods within their business logics. The drawback of this decision, is that binder need the class applying the mixin to provide the scope, and the only solution I found was to declare it as abstract in the Logic mixin, so that classes have to provide it. Because it involves some boilerplate, I created some vscode snippets, to make the creation of a business logic less cumbersome. You can find the snippets here: https://marketplace.visualstudio.com/items?itemName=romain-rastel.flutter-binder-snippets.

But to answer to your question, I don't find any reason for you, to use it directly. You should use the utility methods πŸ˜‰ .

What happens if I initialize a reference inside a widget? All the examples show the use of LogicRef and StateRef being global variables. I was wondering: is it a problem if one is initialized as a widget attribute? There is no risk of memory leak or callback called several times?

LogicRefs and StateRefs stand for ids. If you initialized them as a widget attribute, then, they can be recreated each time the parent widget rebuilds and would inevitably lead to issues. And yes I think it can cause memory leaks indeed, if the BinderScope is not cleaned. If you don't want everyone outside a file to be able to watch a StateRef you can declare it globally but private. Thanks for raising this, I need to add something in the README to inform the users!

Is there any reason why it is not possible to call context.read() for a Computed? In particular, if one wants to access the value in a callback. It's possible to use context.read() on a StateRef but not on a Computed (which I thought would be similar to a StateRef as it is derived from). Why that?

It looks like I made a mistake πŸ‘ . Everything is plugged in so that we can do it, except for the extensions and Logic. I need to look at it! Thanks πŸ™‚ !

What is the best way to request data while building a widget? Sometimes, my View or Widget does not depend on an actual state but rather on temporal data. For example, when I need to display a list of values loaded from the cloud. How do you usually load and store this ephemeral data from a LogicRef? I do something similar to this...

I would not use a FutureBuilder because the future will be called every time the widget rebuilds, which is maybe not what you want. Instead you can create a StatefulWidget and load the data in the initState. I've done it in several examples. You can see such a widget here: https://github.com/letsar/binder/blob/main/examples/architecture/lib/core/widgets/logic_loader.dart I didn't yet added one in binder because I want to see what is the best API to provide for this feature. If you have an idea, your feedback will be much appreciated.

How to handle the lifetime of one or more Logic "instances"?

Interesting use case. This is the right thing to do. Simply create a BinderScope and provide it the overrides it needs to have its own logic and states πŸ™‚ ! When the BinderScope will be disposed, all the overriden states and logics in this scope will be deleted as well.

Would you say that this is the idiomatic way to handle the Logic as if it were a class instance? I mean, the logic and states do not actually need to exist by default. They can be instantiated independently one or multiple times, and they should "die" when the view no longer use it.

YES and YES πŸ™‚ ! Logics are instantiated only in overrides or when we use them for the first time. And they "die" when the BinderScope, holding their state is disposed. So if we want a logic instance to be temporal, we have to override it in a new BinderScope. For logics, if the factory is the same, you can use overrideWithSelf instead of providing the same factory in another overrideWith.

For example, let's say chessGameLogicRef is declared like that:

final chessGameLogicRef = LogicRef((scope) => ChessGameLogic(scope));

instead of writing this:

chessGameLogicRef.overrideWith((scope) => ChessGameLogic(scope)),

you can write this:

chessGameLogicRef.overrideWithSelf(),

I'm not totally happy with how I named overrideWithSelf, so if you have a better suggestion, I'm all ears πŸ™‚ .

I hope to have answer to all your questions. If you're still happy with binder, don't hesitate to share your feedback with others. The more people use it and the more feedback I get, the more I can adjust the API and make the documentation better πŸ™‚ .

letsar commented 4 years ago

I've just pushed v0.2.1 in which you can use any watchable with context.read, not just a StateRef πŸ˜‰ .

Delgan commented 4 years ago

Hey! Thanks for the quick answers and the fast new release! :tada:

I didn't want business logics to directly inherit from a type coming from binder. Otherwise I would have find binder too much intrusive. Instead I decided to go with a mixin, which allows the user to call the utility methods within their business logics.

It's an interesting design consideration that I've never thought of before. That makes a lot of sense. Anyway, I'm already using the binder snippets you published so it's not a problem.

I would not use a FutureBuilder because the future will be called every time the widget rebuilds, which is maybe not what you want. Instead you can create a StatefulWidget and load the data in the initState.

Oups, yeah. Classic pitfall of FutureProvider. In this sense, having a utility class or a documented example would certainly be helpful. I don't know exactly what would be the best API, my use cases have been the following:

For the last two cases (which are actually identical, modulo async), I mean that the widget will use the data directly instead of going through a StateRef. Such use case would require adapting the LogicLoader so that it calls some initializer during initState() and forwards the returned value to a builder. However, it requires using .addPostFrameCallback() which means the value will not yet be available during first build(). Also, using a Future is "sub-optimal" when the Widget just needs to obtain the value synchronously.

I guess it's a little bit ambiguous so let me add an example. :)

final fetchedDataCountRef = StateRef(0);

final dataFetcherRef = LogicRef((scope) => DataFetcher(scope));

class DataFetcher with Logic {
  const DataFetcher(this.scope);

  @override
  final Scope scope;

  int fetch(int num) {
    update<int>(fetchedDataCountRef, (count) => count + 1);
    return num * 2;
  }
}

class MyData extends StatefulWidget {
  final int num;

  const MyData(this.num);

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

class _MyDataState extends State<MyData> {
  int data;

  @override
  void initState() {
    super.initState();
    data = context.use(dataFetcherRef).fetch(widget.num);
  }

  @override
  Widget build(BuildContext context) => Text("Data: $data");
}

This code is buggy: if I call data = context.use(dataFetcherRef).fetch(widget.num); this generates an error "setState() or markNeedsBuild() called during build." (because of the update()). If I use addPostFrameCallback() to fetch the value then data is null. So I'm not sure how to solve this properly, apart from using a Future.

YES and YES slightly_smiling_face ! Logics are instantiated only in overrides or when we use them for the first time. And they "die" when the BinderScope, holding their state is disposed. So if we want a logic instance to be temporal, we have to override it in a new BinderScope.

Great! It looks like a very elegant solution for my multiple Logic instances problem. I like how overrideWith() naturally fits this use case.

For logics, if the factory is the same, you can use overrideWithSelf instead of providing the same factory in another overrideWith.

Good to know, thanks. That's certainly better than repeating (scope) => ChessGameLogic(scope). The name overrideWithSelf is pretty descriptive, I don't have many ideas to improve it.

So, yeah, thank you again for clarifying my concerns. I hope binder will encounter the success it deserves. :+1:

letsar commented 4 years ago

Thank you for the interest you have in this package and for your well written comments.

Concerning the LogicLoader widget, I need to go through more examples to see what is the best way to do it.

Thank you for the example. That was a use case I didn't think about. Do you care about fetchedDataCountRef to be updated on the same frame, or it does not matter? How did you do it with Provider or any other state management package? I'm not sure how to handle this use case, because changing the state will inevitably calling a rebuild.

In your use case, do you really count and display the number of times the app fetch data, or was it only to explain the concept?

Thanks for your kind words, I also hope this package will find an audience πŸ™‚ .

Delgan commented 3 years ago

Do you care about fetchedDataCountRef to be updated on the same frame, or it does not matter?

No, I think it does not matter. Actually, I never had to make a widget which is dependent on when the frame is refreshed. The logic notifies that some state changed, and the view will update whenever it can.

How did you do it with Provider or any other state management package?

Without using binder, I would do something like that:

class DataBloc {
  final fetchedCount = ValueNotifier<int>(0);

  int fetch(int num) {
    fetchedCount.value += 1;
    return num * 2;
  }
}

class DataWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final data = Provider.of<DataBloc>(context).fetch(111);
    return Column(children: [
      ValueListenableBuilder<int>(
        valueListenable: Provider.of<DataBloc>(context).fetchedCount,
        builder: (context, count, _) => Text("Fetched count: $count"),
      ),
      Text("Fetched data: $data"),
    ]);
  }
}

class DataView extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Provider<DataBloc>(
      create: (_) => DataBloc(),
      child: DataWidget(),
    );
  }
}

Well, of course .fetch() will be called at each rebuild, which may be undesirable. In such case, I would simply define a new widget and call it from inside the initState().

In your use case, do you really count and display the number of times the app fetch data, or was it only to explain the concept?

This was a purely fictional example. :slightly_smiling_face:

The issue I faced came from a StateHolder widget I implemented. It accepts a callback, the widget calls the callback during initState() and stores the returned value, but without using addPostFrameCallback(). Also, I have a Logic which updates some StateRef during .init(). While using this widget to initialize my Logic, I realized it caused an error because of update() during Logic.init().

StateHolder<void>(
    initialize: (context) => context.use(myLogicRef).init(),
    builder: (context, state) => ...,                       
)

Thanks to this widget, the idea was to call context.use(myLogicRef).fetch(111) during initialize so that state equals 222 at first build.

I think maybe I'm going in the wrong direction. A possible better approach would be refactoring the code so that 111 and data are associated with a StateRef. Perhaps the view should not request for data this way.

On the other hand, sometimes it's very convenient just to let the view somehow call the logic. For example : a widget which displays a pseudo-random number generated by a Logic. While generating the number, the Logic needs to read() and update() different StateRef for some reason. The widget should be built only once. Displaying a default value (like 0) from one frame before re-building the widget with the generated number is undesirable. The widget may be used multiple times in the view. Using binder, I would not know how to implement that.

letsar commented 3 years ago

Ok I see. For this kind of use case we need to do something special. Because binder is based on InheritedModel, a rebuild is requested each time we change a value. For this use case we need to workaround this. One solution could be to also work with a ValueNotifier like you did with provider:


final dataLogicRef = LogicRef((scope) => DataLogic(scope));

final fetchedCountRef = StateRef(ValueNotifier<int>(0));

class DataLogic with Logic {
  DataLogic(this.scope);

  @override
  final Scope scope;

  int fetch(int num) {
    read(fetchedCountRef).value++;
    return num * 2;
  }
}

class DataWidget extends StatefulWidget {
  @override
  _DataWidgetState createState() => _DataWidgetState();
}

class _DataWidgetState extends State<DataWidget> {
  int data;
  @override
  void initState() {
    super.initState();
    data = context.use(dataLogicRef).fetch(111);
  }

  @override
  Widget build(BuildContext context) {
    return Column(children: [
      ValueListenableBuilder<int>(
        valueListenable: context.watch(fetchedCountRef),
        builder: (context, count, _) {
          return Text("Fetched count: $count");
        },
      ),
      Text("Fetched data: $data"),
    ]);
  }
}

I prevented to call use inside build methods to avoid some issues, but maybe I should remove this restriction.

Delgan commented 3 years ago

Hum, yeah, thanks for the workaround. It's a possibility I hadn't thought of. However, it would bother me a bit. Wrapping a ValueNotifier in a StateRef and using ValueListenableBuilder makes binder lose its awesomeness!

If I were to implement a workaround, I think I would simply postpone the update.

final fetchedCountRef = StateRef(0);

final dataFetcherRef = LogicRef((scope) => DataFetcher(scope));

class DataFetcher with Logic {
  const DataFetcher(this.scope);

  @override
  final Scope scope;

  int fetch(int num) {
    Future.microtask(() => update<int>(fetchedCountRef, (count) => count + 1));
    return num * 2;
  }
}

class MyData extends StatefulWidget {
  final int num;

  const MyData(this.num);

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

class _MyDataState extends State<MyData> {
  int data;

  @override
  void initState() {
    super.initState();
    data = context.use(dataFetcherRef).fetch(widget.num);
  }

  @override
  Widget build(BuildContext context) {
    final count = context.watch(fetchedCountRef);
    return Column(children: [
      Text("Fetched count: $count"),
      Text("Fetched data: $data"),
    ]);
  }
}

I don't think you need to allow use() in build methods. At least personally, I prefer to use a workaround for the few times I need it, rather than having less restrictions but opening the door to a whole new set of mistakes.

I don't know precisely what are all the possible pitfalls of using context.use() in a build method, but in my case, it's probably not the best solution. The build() method can be called a arbitrary number of times by the framework, using logic within it is therefore not a good idea. Hence the importance of initState() as you suggested.

In fact, the main problem is the following: it is not possible to use Logic.write() inside initState().

If we solve this, then my use case "let the view request data from logic" is solved too without needing a workaround. It will also prevent anyone from possibly getting an error while using a Logic in initState() without calling addPostFrameCallback().

For example, I made this change t binder which seems to solve the problem, but I don't know anything about the other possible consequences.

diff --git a/packages/binder/lib/src/binder_scope.dart b/packages/binder/lib/src/binder_scope.dart
index c409936..90d27ed 100644
--- a/packages/binder/lib/src/binder_scope.dart
+++ b/packages/binder/lib/src/binder_scope.dart
@@ -157,9 +157,8 @@ class BinderScopeState extends State<BinderScope>
     if (isOwner(ref.key)) {
       void applyNewState() {
         readOnlyKeys.remove(ref.key);
-        setState(() {
-          states[ref.key] = state;
-        });
+        states[ref.key] = state;
+        SchedulerBinding.instance.addPostFrameCallback((_) => (context as Element).markNeedsBuild());
       }

       final effectiveObservers = [...observers, ...widget.observers];

What do you think? I guess this is what you had in mind when you asked me if it mattered that fetchedDataCountRef was updated during the same frame.

Ideally, we should not postpone the setState() if we are already outside of initState(). I don't know how addPostFrameCallback() behaves in this case, and I'm not sure if it can be detected.

letsar commented 3 years ago

Yes this is not a full binder solution. But I'm not sure I can find one if we want to have the fetchCount on the same frame. I will think about that though.

Yes, the solution with the Future.microtask was what I add in mind when I asked :

Do you care about fetchedDataCountRef to be updated on the same frame, or it does not matter?

Yes we can only mutate data outside build because it requests a rebuild (if the state has changed). Sorry, I cannot apply the change you proposed because it would otherwise always make the change one frame after, and since it's not a typical use case, it would be penalizing. If more people are interested by this use case, I could do something to let the user choose, but it would complicate a little the API and the code.

In the mean time I keep in mind this to see if we could do something better.

Delgan commented 3 years ago

Thanks for the time you spent on my problems! I'm closing this issue because you offered a solution for all the questions I had. :+1:

If more people are interested by this use case, I could do something to let the user choose, but it would complicate a little the API and the code.

Yeah, I don't think it's worth complicating the API for that. The ideal solution would be transparent for the user, so that we can develop with a mind freed from this edge case. Otherwise, I can just use one of the workarounds discussed here.

I will open tomorrow an issue specifically about the "calling Logic.write() during initState()" use case. It will be clearer.

I was afraid that addPostFrameCallback() would be undesirable for this reason. I was not sure, as I don't know when usual callbacks like onPressed (where Logic is primarily used) are handled by the framework.

Alternatively, I thought we could detect if we are inside initState() and use addPostFrameCallback() only if necessary. I was hoping you had an idea about this, but unfortunately it doesn't seem possible...

It's very frustrating because I feel like there's not much missing to have a perfect solution!