escamoteur / watch_it

MIT License
103 stars 8 forks source link

Discussion about WatchIt API, the sucessor of get_it_mixin #1

Closed escamoteur closed 1 year ago

escamoteur commented 1 year ago

Hi friends,

I tagg you here because you were involved with the get_it_mixin in the past I'm very interested in your thoughts in a new API that has the chance to get as popular as get_it @esDotDev @dancamdev @cliftonlabrum @kekland @franzlst @Ruzo @lukepighetti @albertodev01

See the previous discussion here https://github.com/fluttercommunity/get_it/issues/246

I already have refactored the get_it_mixin to now use a global variable like hooks and added two WidgetTypes.

It also publishes a global GetIt instance called di which is the reason why I propose to omit the previous get and getX functions (the latter was admittedly a bit childish joke on the getX package)

Now there are different ways we realize the watch functions.

1 We reduce the functions of WatchIt to basically two watch functions (the handler and stream and future watches can stay as they are more or less) one to watch a ValueListenable and one for ChangeNotifiers that will trigger a rebuild but are not automatically connected with get_it. this could look like this then:

loading = watchValue(di<UserManager>().loading).value;

or

loading = watch(di<UserManager>()).loading;

This definitely reduces the number of generic parameters but would it make impossible to update only when one property of a ChangeNotifier changes.

2 We keep the direct link to GetIt like in the get_it_mixin but make the usage easier by renaming and splitting some of the methods into separate more specialized ones instead of one with a dozen parameters.

Before proposing a full new API let's discuss which functions we think to make sense that observe

  1. Simple Listenables inside GetIt or just local ones. Optional they could have a select or only accessor to properties of that Listenable and trigger a rebuild only when the value of that property changes. I'm not sure if we should keep the variant to observe a property that is a member of a Listenable that is a property of an object inside get_it like the old watchXOnly because that could be done with the local watch function and manual access to that property using watch(di<ParenObjectType>().ListenableProperty).

  2. ValueListenables inside GetIt, just local ones or ValueListenable properties of an Object inside GetIt

Some functions of the current get_it_mixin library support the scenario to observe a local object with an optional target parameter but I feel that is too hidden and complicates the API. Therefore I feel that these should be separate watch functions.

The current watch functions all need a clever positioning of the types or providing a lot of generic types, especially the watchX functions that need a type to access the object and to directly return the value of the value property. That could be simplified by just returning the ValueListenable so that the user has to add a .value behind the watch function. Possibly this could be solved by using dynamic type information when looking up the object inside get_it. I would have to dig into that a bit more.

So please let me know your thoughts on this and on how we should name the new functions.

esDotDev commented 1 year ago

I wonder if there is some way this would all be combined into a single method with an optional select call.

The watch would handle the lookup (which is option #2 above), then select can be used as a selector when needed, to retrieve any arbitrary value, or the contents of a value notifier:

/// listen to listenable singleton
final bar = watch<FooNotifier>().bar;

/// listen to any field on a listenable singleton
final bar = watch<FooNotifier>(select: (f) => f.bar);

/// bind to a Listeneable field on a singleton
final bar = watch<Foo>(select: (f) => f.barNotifier);

Not sure this is possible without the generics getting verbose, but maybe there is some food for thought. I think the watch/select pattern is strong.

We would probably need some runtime asserts, as either the watched object, or the results of select need to be a listenable.

escamoteur commented 1 year ago

Yes, indeed, that would be nice but I'm not sure if that is possible in any type safe way. There is already an open PR on GetIt to lookup an object by passing a Type at runtime. That could possibly help. It's pretty sad that you can't just provide one generic and let the compiler deduct the rest of them

What about watching a local Object?

dancamdev commented 1 year ago

Expanding on @esDotDev - what if the select function can only take an object that extends Listenable?

Function declaration looking something like:

void Function(Listenable) select

Both ChangeNotifier and ValueListenable extend Listenable - so it should incur in nested generics.

escamoteur commented 1 year ago

But in the version where the select is used to observe a property of a Listenable the property can be any type, not just a Listenable. Maybe we should distinguish between watch and watchProperty and watchIt for local objects?

escamoteur commented 1 year ago

Question: do you think when watching an ValueListenable it is acceptable to have the user add the .value himself?

final bar = watch<Foo>(select: (f) => f.barNotifier).value;

I'm trying to find a solution that doesn't require it.

escamoteur commented 1 year ago

Ok, it seems that we can fulfill the above proposals almost completely with this version of watch

R watch<T extends Object, R>(
    {T? target, dynamic Function(T)? select, String? instanceName});

The only thing that is impossible is to avoid passing in R either directly or by assigning the receiving variable a type. So the above would look like

/// listen to listenable singleton
final bar = watch<FooNotifier,FooNotifier>().bar;
/// if its a local Listenable
final BarType bar = watch(target: fooNotifier).bar;

/// listen to any field on a listenable singleton
final bar = watch<FooNotifier,BarType>(select: (f) => f.bar);
/// or
final BarType bar = watch(select: (FooNotifier f) => f.bar);
/// or with a local listenable
final BarType bar = watch( target: fooNotifier, select: (f) => f.bar);

/// bind to a ValueListeneable field on a singleton Foo
final bar = watch<Foo,BarType>(select: (f) => f.barNotifier);
// or
final BarType bar = watch(select: (Foo f) => f.barNotifier);
// or to a local ValueListenable
final BarType bar = watch(target: barNotifier);

That's not too bad but having the need to provide the type R because of the goal of directly returning the value of a ValueListebable is not really nice. I see three options:

  1. watch does only return the ValueNotifier, the user has to add a .value
  2. remove R and return a dynamic which might be the most comfortable but can lead to runtime errors if the receiving variable is not the correct type
  3. we split the above function into two separate in separate functions
R watch<T extends Object, R>(
    {T? target, dynamic Function(T)? select, String? instanceName});
T watchListenable<T extends Listenable>{T? target, T Function(T)? select, String? instanceName});

what do you think?

may I also pick your brain @munificent

cliftonlabrum commented 1 year ago

I'm not as advanced as all of you with this stuff, but what is the difference between listening to a singleton like this:

final bar = watch<FooNotifier,FooNotifier>().bar;

...and listening to a property of a singleton like this?

final bar = watch<FooNotifier,BarType>(select: (f) => f.bar);

Do they not do the same thing? If they do, the first line is preferable.

The most concise version I've seen to watch a class property (and easiest to understand for a get_it beginner like me) is this:

loading = watch(di<UserManager>()).loading;

In my opinion, the two most common use cases are:

  1. Watch for changes on a single property on a class
  2. Watch if any property on a class changes

I wouldn't use much beyond that—except for an occasional ValueNotifier like final someValue = ValueNotifier<double>(0.0); in one of my model classes.

escamoteur commented 1 year ago

@cliftonlabrum, the difference between the two above is that the first one always triggers a rebuild when FooNotifier's any property of it changes and triggers a notifyListener. The second one only triggers a rebuild if the property bar changes its value and it will ignore any other property changes.

loading = watch(di<UserManager>()).loading;

would indeed be the most open version, but requires the additional di.

Thanks for your thoughts. Curious what others think

kekland commented 1 year ago

@escamoteur I feel like the concept of having a single function is good, but IMO having to provide types is a bit cumbersome (especially in a case where you have to watch a singleton in the DI).

What I have in mind is: have a watch() function which returns an object of type WatchResult, from which you can do three things:

  1. watch().value - to observe the object directly
  2. watch().select((foo) => foo.myListenable) - to observe some ValueListenable property in that object
  3. watch().select((foo) => foo.bar) - to observe some property in that object

In practice, it will look like the following:

final target = MySingleton();

final singleton = watch<MySingleton>().value; // Type is MySingleton

final singletonTarget = watch(target: target).value; // Type is MySingleton

final counter = watch<MySingleton>().select((v) => v.counter); // Type is int

final bar = watch<MySingleton>().selectProperty((v) => v.bar); // Type is int

I feel like the benefit of not having to provide any types explicitly (unless absolutely necessary - in this case for DI) would be huge for me personally, as this would make the resulting code cleaner.

One trade-off of this method is that for observing a singleton, now you would have to append .value at the end of the .watch() function.

As for having a singular .select() function (for both value listenables and regular fields) - I'm not sure whether it's possible to provide type inference. And, personally, I feel like that separating it into two functions (for listenables and for properties) is fine, because in most cases .select() would be used to select a value listenable directly.

Here's the full code for my proposal:


class WatchResult<T> {
  WatchResult(this.target, this.instanceName);

  final T? target;
  final String? instanceName;

  T get _target => target ?? di<T>(instanceName: instanceName);

  T get value {
    // Listen to the [_target] here.
    return _target;
  }

  R select<R>(ValueListenable<R> Function(T) selector) {
    final property = selector(_target);

    // Listen to the property here.
    return property.value;
  }

  R selectProperty<R>(R Function(T) selector) {
    final property = selector(_target);

    // Listen to the [_target] here.
    return property;
  }
}

WatchResult<T> watch<T>({T? target, String? instanceName}) {
  return WatchResult(target, instanceName);
}

class MySingleton {
  final ValueNotifier<int> counter = ValueNotifier(0);

  var bar = 1;
}

void build() {
  final target = MySingleton();

  final singleton = watch<MySingleton>().value; // Type is MySingleton
  final singletonTarget = watch(target: target).value; // Type is MySingleton
  final counter = watch<MySingleton>().select((v) => v.counter); // Type is int
  final bar = watch<MySingleton>().selectProperty((v) => v.bar); // Type is int
}
escamoteur commented 1 year ago

@kekland That is really an interesting approach. Actually, I was also already on the way to accepting that the user has to add a .value on ValueListenables . Will have a closer look at how we could implement that.

lukepighetti commented 1 year ago

Just a quick note I've been building a lot of this functionality on the fly in a side project and I've pretty much given up on selectors because I have yet to see them provide real performance benefits. That said they should probably be available as an escape hatch. I think ValueNotifier should be the default use case for ValueNotifier<T> watch<T>(ValueNotifier<T>) there's an opportunity to convert ChangeNotifier / Listenable to a ValueListenable with an extension with an optional selector. I think the two way binding provided by ValueNotifiers are important to preserve, but you can probably add some cheap safety by having a method that returns just a ValueListenable

munificent commented 1 year ago

I don't have a lot of context for this discussion, but splitting it into two functions so that you don't have to write type arguments that feel pointless in some situations seems reasonable to me.

escamoteur commented 1 year ago

I now implemented two versions of a possible API https://github.com/escamoteur/watch_it/tree/watchResult

here is an impression to compare how this looks in the test widget: image and https://github.com/escamoteur/watch_it/tree/multiple-functions image

I will write a more detailed comparison in the next few days but I'm curious about what you prefer

escamoteur commented 1 year ago

@esDotDev @dancamdev After looking a bit more at the two options I personally lean toward the more traditional approach with separate functions. What are your thoughts?

dancamdev commented 1 year ago

I'm missing a lot of the implementation context needed to give you a fair opinion Thomas - I do like the second approach as well, but wondering if it's possible to further simplify the API on the first one using extensions, getting closer to the second approach.

escamoteur commented 1 year ago

OK, I added some documentation to the library file so I hope the above second example makes more sense:

/// WatchIt exports the default instance of get_it as a global variable which lets
/// you access it from anywhere in your app. To access any in get_it registered
/// object you only have to type `di<MyType>()` instead of `GetIt.I<MyType>()`
/// if you want to use a different instance of get_it you can pass it to the
/// the functions of this library as an optional parameter
final di = GetIt.I;

/// The Watch functions:
///
/// The watch functions are the core of this library. They allow you to observe
/// any Listenable, Stream or Future and trigger a rebuild of your widget whenever
/// the watched object changes.
/// To be able to use the functions you have either to derive your widget from
/// [WatchingWidget] or [WatchingStatefulWidget] or use the [WatchItMixin] in your
/// widget class.
///
/// To use the watch functions you have to call them inside the build function of
/// a [WatchingWidget] or [WatchingStatefulWidget] or a class that uses the
/// [WatchItMixin]. They basically allow you to avoid having to clutter your
/// widget tree with `ValueListenableBuilder`, `StreamBuilder` or `FutureBuilder`
/// widgets. Making your code more readable and maintainable.

/// Observes any Listenables and triggers a rebuild whenever it notifies
/// a change.That listenable could be passed in as a parameter or be accessed via
/// get_it. Like `final userName = watch(di<UserManager>()).userName;` if UserManager is
/// a Listenable (eg. ChangeNotifier).
/// if any of the following functions don't fit your needs you can probably use
/// this one by manually providing the Listenable that should be observed.
T watch<T extends Listenable>(T target);

/// Observes any Listenable registered in get_it and triggers a rebuild whenever
/// it notifies a change. Its basically a shortcut for `watch(di<T>())`
/// [instanceName] is the optional name of the instance if you registered it
/// with a name in get_it.
/// [getIt] is the optional instance of get_it to use if you want to use the
/// default one. 99% of the time you won't need this.
T watchIt<T extends Listenable>({String? instanceName, GetIt? getIt});

/// Observes a ValueListenable property of an object regisertered in get_it
/// and triggers a rebuild whenever it notifies a change and returns the current
/// value of the property. Its basically a shortcut for `watchIt<T>().value`
/// As this is a common scenario it allows us a type safe concise way to do this.
/// `final userName = watchValue<UserManager, String>((user) => user.userName);`
/// is an example of how to use it.
/// We use the strength of generics to infer the type of the property and write
/// it even more expressive like this:
/// `final userName = watchValue((UserManager user) => user.userName);`
///
/// [instanceName] is the optional name of the instance if you registered it
/// with a name in get_it.
/// [getIt] is the optional instance of get_it to use if you want to use the
/// default one. 99% of the time you won't need this.
R watchValue<T extends Object, R>(ValueListenable<R> Function(T) selectProperty,
    {String? instanceName, GetIt? getIt});

/// Allows you to onbserve a property of a Listenable object and trigger a rebuild
/// whenever the Listenable notifies a change and the value of the property changes and
/// returns the current value of the property.
/// You can achie a similar result with `watchIt<UserManager>().userName` but but that
/// would trigger a rebuild whenever any property of the UserManager changes.
/// `final userName = watchProperty<UserManager, String>((user) => user.userName);`
/// could be an example. Or even more expressive and concise:
/// `final userName = watchProperty((UserManager user) => user.userName);`
/// which lets tha analyzer infer the type of T and R.
///
/// if you have a local Listenable and you want to observe only a single property
/// you can pass it as [target].
/// [instanceName] is the optional name of the instance if you registered it
/// with a name in get_it.
/// [getIt] is the optional instance of get_it to use if you want to use the
/// default one. 99% of the time you won't need this.
R watchProperty<T extends Listenable, R>(R Function(T) selectProperty,
    {T? target, String? instanceName, GetIt? getIt});

/// subscribes to the `Stream` returned by [select] and returns
/// an `AsyncSnapshot` with the latest received data from the `Stream`
/// Whenever new data is received it triggers a rebuild.
/// When you call [watchStream] a second time on the same `Stream` it will
/// return the last received data but not subscribe another time.
/// To be able to use [watchStream] inside a `build` function we have to pass
/// [initialValue] so that it can return something before it has received the first data
/// if [select] returns a different Stream than on the last call, [watchStream]
/// will cancel the previous subscription and subscribe to the new stream.
/// [preserveState] determines then if the new initial value should be the last
/// value of the previous stream or again [initialValue]
/// if you want to observe a `Stream` that is not registered in get_it you can
/// pass it as [target].
/// if pass null as [select] T or [target] have to be a Stream<R>.
/// [instanceName] is the optional name of the instance if you registered it
AsyncSnapshot<R> watchStream<T extends Object, R>(
  Stream<R> Function(T)? select, {
  T? target,
  R? initialValue,
  bool preserveState = true,
  String? instanceName,
  GetIt? getIt,
})

I stop here, as the principle should be clear by now I think. There are additional watch functions for Futures and to register Handlers for Listenable, Streams, and Futures that are called instead of triggering a rebuild which allows to react on events in StatelessWidget like in a Stateful one.

escamoteur commented 1 year ago

@dancamdev I hope that helps :-) And I'm curious if you still think the API is too complecated :-)

esDotDev commented 1 year ago

I think I personally prefer the multiple methods approach, as it makes the usage cleaner. watchProperty((Model x) => x.country); vs watchIt<Model>().selectProperty((x) => x.country);

I would rather write the first version inside of my views. But then I think we are back into the issue of ambiguous names. It's not exactly clear what the difference is between watchValue vs watchProperty, we're sorta back to watchOnly vs watchX where you need to memorize what these really mean.

I think maybe more clear names is all we need? eg watchListenable watchListenableField watchValueNotifier

We give up some characters here, but this would be auto-completed in one-stroke by IDE, and should help readability quite a bit?

esDotDev commented 1 year ago

In terms of using extension methods, could something be done where getIt is still used directly, but then watch extensions are simply added onto the classes as needed?

// For changeNotifiers, we'd have `watch` and `watchProp` extensions
getIt<AppModel>().watch();
getIt<AppModel>().watchProp((m) => m.user.value);

// For ValueNotifiers we have a `watch` extension
getIt<AppModel>().user.watch(); 

// Similarly for streams:
getIt<AppModel>().someStream.watch(); 

This is similar to option 1 above, but removes some extra boilerplate (I think?)

escamoteur commented 1 year ago

Interesting, I was playing with the chained methods and I don't think that it's more obvious that you need to call a .value to make it watch a simple listenable.

To naming :

to me a function isn't only qualified by its name but also by its signature. In c# which allows operator overloading all of these functions would have the same name without being less understandable

watch - just watch any listenable watchIt - watch a listenable inside GetIt watchValue there the ValueListenable can be guessed by the Value and one look in the signature should eliminate any questions watchProperty (watchSelected) is also not that far fetched. Definitely better than watchX or watchOnly

Am 31. Mai 2023, 18:55 +0200 schrieb Shawn @.***>:

I think I personally prefer the multiple methods approach, as it makes the usage cleaner. watchProperty((Model x) => x.country); vs watchIt().selectProperty((x) => x.country); I would rather write the first version inside of my views. But then I think we are back into the issue of ambiguous names. It's not exactly clear what the difference is between watchValue vs watchProperty, we're sorta back to watchOnly vs watchX where you need to memorize what these really mean. I think maybe more clear names is all we need? eg watchListenable watchListenableField watchValueNotifier We give up some characters here, but this would be auto-completed in one-stroke by IDE, and should help readability quite a bit? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

escamoteur commented 1 year ago

In terms of using extension methods, could something be done where getIt is still used directly, but then watch extensions are simply added onto the classes as needed?

// For changeNotifiers, we'd have `watch` and `watchProp` extensions
getIt<AppModel>().watch();
getIt<AppModel>().watchProp((m) => m.user.value);

// For ValueNotifiers we have a `watch` extension
getIt<AppModel>().user.watch(); 

// Similarly for streams:
getIt<AppModel>().someStream.watch(); 

This is similar to option 1 above, but removes some extra boilerplate (I think?)

this is actually an interesting third option I didn't have on my mind yet. have to check if we get any limitation from it.

escamoteur commented 1 year ago

Two things I don't like with this approach, with extension functions on Listenable, ValueListenable, Stream, and Future we pollute the intellisense space and might make people call these extensions outside the build function more easily. It also moves the focus from watching something away as it's just some extension function at the end of an expression

esDotDev commented 1 year ago

Ya, those are fair points, but the usage is still very nice (not worrying about method names, just call watch). I'm not sure the emphasis thing is really a big deal, but I do see what you mean, kinda nice if we lead with watch... from a readability POV.

As you say, method overloading would be quite nice here :'(

escamoteur commented 1 year ago

I just added this list to the code comments and I think seeing it like this makes a lot of sense image

esDotDev commented 1 year ago

Yes, that makes it easier to grok. I think what's bugging me is the watchProp actually checks the value, while the watchValue does not, and just relies on a notify call, so the name still strikes me as confusing.

Maybe a good direction to go from here is to brainstorm alternative names for watchProperty and watchValue and see if there are some other more clear options?

Maybe something like watchValueNotifier and watchPropertyValue just to be super explicit / clear?

escamoteur commented 1 year ago

It would have to be watchValueListenable then which seems a monster of a name like ValueListenableBuilder. I like watchPropertyValue though.

p.s.: have you seen my comment https://github.com/fluttercommunity/get_it/issues/326#issuecomment-1567932439 ?

escamoteur commented 1 year ago

I extended the intro docs a bit https://github.com/escamoteur/watch_it/blob/77235e66c2d365f3d238131e9ee9583eb8af1c05/lib/src/watch_it.dart#L13-L185

escamoteur commented 1 year ago

@dancamdev what do you think about the above proposal? @esDotDev if you can live with the above API, I will publish a prerelease soon

esDotDev commented 1 year ago

Well just my 2cents, but I'd I prefer a long name (which is auto-completed anyways) if it provides more clarity and makes things a little more readable. My main concern is the vagueness of watchIt it's not clear to the reader what it is.

I think I'd opt for: watchListenable watchListenableProperty watchValue

escamoteur commented 1 year ago

Interesting. I asked several people for feedback and most of them said watchIt is wachting something inside getIt. Am 9. Juni 2023, 18:26 +0200 schrieb Shawn @.***>:

Well just my 2cents, but I'd I prefer a long name (which is auto-completed anyways) if it provides more clarity and makes things a little more readable. My main concern is the vagueness of watchIt it's not clear to the reader what it is. I think I'd opt for: watchListenable watchListenableProperty watchValue — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

esDotDev commented 1 year ago

Sure, that much is probably clear, but it's not clear (to me) that the thing it is watching is a ChangeNotifier/Listenable

escamoteur commented 1 year ago

As said before the signature is as important as the name itself. Am 9. Juni 2023, 19:03 +0200 schrieb Shawn @.***>:

Sure, that much is probably clear, but it's not clear (to me) that the thing it is watching is a ChangeNotifier/Listenable — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

esDotDev commented 1 year ago

Ya that's fair, my perspective is more on the reader than the writer. In one case it's clear from the name what it is doing, in another you must mouse-over in order to inspect it, you can't simply scan it visually (well you can, if you memorize what it is)

escamoteur commented 1 year ago

BTW watchPropertyValue Was your previous proposal 😁 Am 9. Juni 2023, 19:10 +0200 schrieb Shawn @.***>:

Ya that's fair, my perspective is more on the reader than the writer. In one case it's clear from the name what it is doing, in another you must mouse-over in order to inspect it, you can't simply scan it visually. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

escamoteur commented 1 year ago

On a second thought, I think for a Reader it's even easier. They see any watch and (if they know what watch_it is) they now some listenable is watched and will trigger a rebuild. Additionally they see the uses generic type or parameter. Am 9. Juni 2023, 19:10 +0200 schrieb Shawn @.***>:

Ya that's fair, my perspective is more on the reader than the writer. In one case it's clear from the name what it is doing, in another you must mouse-over in order to inspect it, you can't simply scan it visually. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

esDotDev commented 1 year ago

I think watchListenable is objectively more clear than watchIt but it's obviously your call :) Me personally, I would almost exclusively use watchValue or watchPropertyValue anyways.

escamoteur commented 1 year ago

just published the first version to pub: https://pub.dev/packages/watch_it