s0nerik / context_plus

Convenient BuildContext-based value propagation and observing. Seamlessly integrates with Flutter's built-in observability primitives.
MIT License
32 stars 4 forks source link

watchValue a downgrade? #6

Closed knopp closed 6 months ago

knopp commented 6 months ago

Hi,

I feel like introducing watchValue is a downgrade. What is the practical purpose of returning the listenable from watch?

final value = valueListenable.watch(context)

seems nicer than

final value = valueListenable.watchValue(context);
s0nerik commented 6 months ago

Hey Matej!

I agree with you. To me, it also feels like a downgrade for the most basic case of dealing with ValueListenables. I wanted to keep that API as-is, but just was unable to make it work well together with the new TListenable TListenable.watch() and Listenable.watchOnly(). Let me share how I came to this.

Basically, for context_watch v2.0 I initially wanted the contract to change from:

extension ListenableContextWatchExtension on Listenable {
  void watch(BuildContext context) {
    return;
  }
}

extension ValueListenableContextWatchExtension<T> on ValueListenable<T> {
  T watch(BuildContext context) {
    return value;
  }
}

to this:

extension ListenableContextWatchExtension<TListenable extends Listenable>
    on TListenable {
  TListenable watch(BuildContext context) {
    return this;
  }

  R watchOnly<R>(BuildContext context, R Function(TListenable) selector) {
    return selector(this);
  }
}

extension ValueListenableContextWatchExtension<TListenable extends ValueListenable<T>, T>
    on TListenable {
  T watch(BuildContext context) {
    return value;
  }

  R watchOnly<R>(BuildContext context, R Function(T) selector) {
    return selector(value);
  }
}

And it worked well at runtime, passing all my existing tests, so I didn't notice any problems until I started writing new examples for context_watch v2.0 and noticed that ValueListenable<int>.watch(context) was returning the value as dynamic instead of int and ValueListenable<int>.watchOnly(selector:) parameter was receiving dynamic instead of int as well. I tried different variants of defining the extensions but found out that none of them allowed me to keep the API I wanted initially.

I had to choose between those sets of methods

  1. TListenable TListenable.watch(BuildContext) R TListenable.watchOnly<R>(BuildContext, R Function(TListenable)) T ValueListenable<T>.watchValue(BuildContext) R ValueListenable<T>.watchValueOnly<R>(BuildContext, R Function(T))
  2. void Listenable.watch(BuildContext) R TListenable.watchOnly<R>(BuildContext, R Function(TListenable)) T ValueListenable<T>.watch(BuildContext) R ValueListenable<T>.watchValueOnly<R>(BuildContext, R Function(T))
  3. void Listenable.watch(BuildContext) T ValueListenable<T>.watch(BuildContext) R TListenable.watchOnly<R>(BuildContext, R Function(TListenable))

and I chose option #1 for its assumed better consistency.

Option #2 would've lead to ValueListenable having three "watch" methods: watch(), watchOnly() and watchValueOnly(). In this case, it's weird why there's a single watch() method, but two different `watchOnly()` methods. The reason is explained above.

Option #3 is just an option #2, but without the ValueListenable.watchValueOnly(). This means that if you have something like

final tabNotifier = ValueNotifier(0);

and want the widget to rebuild only when the selected tab switches from/to the second one, you'll have to write this

final isSecondTab = tabNotifier.watchOnly(context, (notifier) => notifier.value == 2);

instead of

final isSecondTab = tabNotifier.watchValueOnly(context, (tab) => tab == 2);

Now that I'm thinking back on it, I'm not sure whether it was the right decision to go with the option #1 as the API surface for ValueListenable has gotten bigger (e.g. since ValueListenable is also Listenable, it now has all four: Listenable.watch(), Listenable.watchOnly(), ValueListenable.watchValue(), ValueListenable.watchValueOnly()), while the profit gained from it might not be worth degrading the most basic ValueListenable.watch() developer experience, but I'm not sure how to re-evaluate this decision properly either.

What is the practical purpose of returning the listenable from watch?

It allows you to access the Listenables value more fluently.

Basically,

_tabController.watch(context);
final tabIndex = _tabController.index;

can now become

final tabIndex = _tabController.watch(context).index;
knopp commented 6 months ago

What is the practical purpose of returning the listenable from watch?

It allows you to access the Listenables value more fluently.

Basically,

_tabController.watch(context);
final tabIndex = _tabController.index;

can now become

final tabIndex = _tabController.watch(context).index;

I thought so, but this seems a slightly worse version of _tabController.watchOnly(context, (t)=>t.index). Not sure if it was worth having to introduce watchValue for.

knopp commented 6 months ago

Also have you consider renaming watchOnly to select? It's a bit shorter and may be more obvious for people used to provider.

s0nerik commented 6 months ago

Also have you consider renaming watchOnly to select? It's a bit shorter and may be more obvious for people used to provider.

I did, but decided to not go with this name. select is too general of a term, making it quite likely to be present as a method defined on a ChangeNotifier-driven state object. watchOnly() is much better in that regard. I've also initially considered a R TListenable.watchValue<R>(BuildContext, R Function(TListenable)), but had to give up on that name as well because of ValueListenable.watchValue().

Another benefit of watchOnly() name is that it will be suggested by the IDE right near the watch(), which will increase to likelihood of the user looking into it and potentially optimizing some extra rebuilds.

s0nerik commented 6 months ago

CleanShot 2024-02-23 at 16 53 33

After collecting some feedback from my peers, it seems like everyone prefers the third option.

I've also looked into the diffs generated by moving from #1 to #3 in the library examples and haven't found a single example where readability would decrease. So, expect context_watch 3.0 soon and dont hurry upgrading to context_watch 2.0.

If anyone finds the watchValueOnly() valuable enough - it's gonna be really easy for them to define such extension themselves. A couple of lines of code, basically.

s0nerik commented 6 months ago

context_watch 3.0 and context_plus 2.0 have been released with the changes described above.