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

Move Listenable, ValueListenable, ChangeNotifier and ValueNotifier to the Dart sdk and add listening methods #55816

Closed
escamoteur opened this issue May 22, 2024 · 22 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection type-enhancement A request for a change that isn't a bug

Comments

@escamoteur
Copy link

There have been several cases where developers had to reimplement these classes because they didn't want a reference to flutter in their package. As none of these classes needs anything from the Flutter SDK but they are all pretty handy it would make sense to move them from the Flutter to the Dart SDK.

At the same time, their API could be greatly improved by giving them a Stream-like listen method:

ListeningSubcription listen(void Function(ListeningSubcription) handler );

and for ValueListenable

ListeningSubscription listen(void Function(T, ListenableSubscription) handler);

to no longer having to use addListener and removeListener where you have to make sure, to store a reference of your handler.
Instead of calling removeListener you call cancel() on the returned subscription object.

The reason why the subscription is passed to the handler function too is that the handler will be able to cancel() the subscription itself if needed.

my package functional_listeners implements this listen function as an extension method and it has proven how much nicer it makes the usage of Listenables

@lrhn lrhn changed the title Move Listenable, ValueListenable, ChangeNotifier and ValueNotifier` to the Dart sdk and add listening methods Move Listenable, ValueListenable, ChangeNotifier and ValueNotifier to the Dart sdk and add listening methods May 22, 2024
@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection type-enhancement A request for a change that isn't a bug labels May 22, 2024
@lrhn
Copy link
Member

lrhn commented May 22, 2024

Could it be a stream, or is the API defined to work synchronously? (Or is it just that it has no errors and no done events?)

I can see the advantage of getting the subscription passed to every callback, it's something stream listeners have work to get access to. (I could see myself add an extension listenWithSubscription helper on streams.)

That said, this is an API that can easily be implemented by a package, and something I'm not personally particularly invested in, so I don't see it becoming a high prioirty. Especially if it's hard to migrate existing Flutter code to the new implementation.

@escamoteur
Copy link
Author

@lrhn to give some context.

interface implementation
Listenable ChangeNotifier
ValueListenable<T> ValueNotifier<T>

They implement the Observer pattern and work synchronously. They are used everywhere in the Flutter Framework and are very popular for state management solutions. My package functional_listener offers rx like extensions for them.

Especially with the advent of Dart on the server people want to use them outside of a Flutter context but can't because they are part of the Flutter SDK. So you have to write your own which isn't compatible to the Flutter one because not even the interfaces are of the Dart SDK. Currently, you are not able to publish any package that uses Listenables that would be usable in a pure Dart program and inside a Flutter app.

Some friends and I spent considerable time optimizing the ChangeNotifier implementation in the past which was merged into Flutter.

I would keep the existing API, maybe make addListener() return a ListeningSubscription to make canceling easier even if people don't want to use the listen() function. But adding the listen()` function would not break any existing apps' code when these types were moved to the Dart SDK especially if they were added to a Dart library that is included by Flutter automatically.

I propose to add some more non-breaking features that I implemented in functional_listener's CustomValueNotifier that might probably others might find useful too.

For ChangeNotifiers

  • expose the number of current listeners

For ValueNotifiers:

From my CustomValueNotifier:

/// Sometimes you want a ValueNotifier where you can control when its
/// listeners are notified. With the `CustomValueNotifier` you can do this:
/// If you pass [CustomNotifierMode.always] for the [mode] parameter,
/// `notifierListeners` will be called every time you assign a value to the
/// [value] property independent of if the value is different from the
/// previous one. This can be useful when you want to trigger a rebuild everytime you
/// received ne data for instance even if the data hasn't changed.
/// If you pass [CustomNotifierMode.manual] for the [mode] parameter,
/// `notifierListeners` will not be called when you assign a value to the
/// [value] property. You have to call it manually to notify the Listeners.
/// Additionally it has a [listenerCount] property that tells you how many
/// Listeners are currently listening to the notifier.

further additions could be

  • optional equality function
  • an enable property that allows to temporarily disable notifications.

@kevmoo might be interested in this too as he recently asked about sharing business logic between Flutter and non Flutter web frameworks

@kevmoo
Copy link
Member

kevmoo commented May 23, 2024

Another option @escamoteur would be to publish this as a package. I don't there is a reason to have them in Dart itself...other than to avoid a package version dance.

@escamoteur
Copy link
Author

escamoteur commented May 23, 2024 via email

@lrhn
Copy link
Member

lrhn commented May 23, 2024

So the problem is that not only are the interfaces declared by the Flutter SDK, it's also used by it, which is why it can't come from a package.

@kevmoo
Copy link
Member

kevmoo commented May 23, 2024

Could we create a package for these interfaces that uses conditional imports to import the flutter versions in flutter dart.library.ui and package-hosted versions in other contexts.

That'd work, right?

@kevmoo
Copy link
Member

kevmoo commented May 28, 2024

...turns out, it won't work because you'd still need a flutter dependency in the pubspec which won't work on the stand-along Dart SDK. 🤔

@matthew-carroll
Copy link

@kevmoo I mentioned on X that I had filed or joined Flutter tickets related to moving Rect, Offset, etc. into packages. I guess I only joined issues because I couldn't find any existing issues filed by me. So I don't have any links for you.

I will offer a couple examples where geometry was relevant.

First, years ago I created a geometry Dart package: https://pub.dev/packages/superdeclarative_geometry - that package couldn't operate in terms of Flutter geometry concepts because there weren't any Dart-level APIs for that.

Second, a year go I was working on direct PDF rendering with Flutter. But we wanted to create a pure Dart PDF parser. Due to the vector-graphics-style encoding of PDFs, the concept of offsets, rectangles, lines, and shapes are baked into the format. The desire to create a pure Dart parser, combined with a Flutter renderer, resulted in other unnecessary translations from a Dart rectangle to a Flutter rectangle, and a Dart offset to a Flutter offset.

These are just two that I remember now in the moment. I think I've run into similar frustrations more times than that.

BTW, a thought on packaging. I think you mentioned something like dart:shared. Maybe that was just an arbitrary name that didn't mean anything. But I would strongly suggest packaging by behavior, not packaging by some arbitrary notion that things are in both Flutter and Dart.

For example, we already have at least two completely unrelated groups of things in Flutter that we'd like in Dart: observables and geometry. I think it would be a mistake to throw both of those in the same package just because they originally came from Flutter. I think it would make a lot more sense to have dart:observerables and dart:geometry, each of which could be versions independently, used independently, and documented independently.

@kevmoo
Copy link
Member

kevmoo commented May 29, 2024

@matthew-carroll – thanks for the context.

Yeah, dart:shared is throw-away. Sadly "versioned independently" doesn't really fly with SDK libraries. But I agree to having clear "homes".

@matthew-carroll
Copy link

Is the SDK definitely the right place? Perhaps this is another wrinkle to at least keep mind. It seems like the primary goal is to be able to use the same APIs in pure Dart and Flutter for various things. This goal doesn't technically require that those things live in the Dart SDK. They could also be Dart team packages on Pub and still meet that goal equally well.

I'm sure there are other implications there, but I just wanted to make it clear that at least for any cases I've been concerned with, I had no need for those things to actually come baked into the language SDK.

@kevmoo
Copy link
Member

kevmoo commented May 29, 2024

@matthew-carroll – right again. Generally, we've REALLY avoided pkg:flutter depending on any other packages.

  characters: 1.3.0
  collection: 1.18.0
  material_color_utilities: 0.11.1
  meta: 1.14.0
  vector_math: 2.1.4

But I guess adding one more that (effectively) NEVER changes would be easier.

@matthew-carroll
Copy link

Are there any objective metrics or direct cost analysis to depending on packages?

I think we can all understand and appreciate that the extreme of Flutter depending on LOTs of packages is bad. We can all imagine a constant churn as dozens of upstream packages change and cause Flutter version rev'ing constantly. We certainly don't want that.

But, similarly, taking a stance of "adding any more packages is necessarily bad/harmful/costly" probably isn't ideal for engineering, either.

For example, Dart-team controlled packages are a completely different beast than community packages. The Dart team, which in many regards is interchangeable with the Flutter team, can make decisions about when those packages are versioned, and can also enforce very strong backwards compatibility and semantic versioning practices. So even if you added 10 more packages from the Dart team, it's not clear at all that the state of affairs would be any worse than they are currently.

This is doubly true if we're talking about adding packages to Flutter which are defined as existing Flutter APIs that are extracted to packages. That code already lives in Flutter, and Flutter already owned the consequences of that code changing. In other words, if you extract something like ChangeNotifier, any possible cause for a change to the ChangeNotifier API would already be considered a breaking change in Flutter, and require versioning Flutter. That cost is exactly the same whether ChangeNotifier sits in Flutter or in a Pub package called dart:observable.

@escamoteur
Copy link
Author

escamoteur commented May 29, 2024 via email

@matthew-carroll
Copy link

@escamoteur reminds me of a related detail. @kevmoo what are your thoughts about the ability to re-export a package that you import?

There are certain vocal developers who think Flutter should solve all sorts of app problems. When pushed for specific reasons why, one reason that came up was that they were frustrated that there's no canonical package for each app requirement, e.g., payments, push notifications, database, etc.

I said that the community should solve this by creating a meta package which makes an opinionated selection of the best mobile app-centric packages in one. But then I realized that technically that can't happen because I don't think we can re-export packages that we imported, right?

If Dart made it possible to re-export the dependency packages then we could provide that meta package aimed at quickly shipping mobile apps. But also, the problem that @escamoteur wouldn't be a problem because Flutter could re-export the observable and geometry packages.

@felangel
Copy link

felangel commented May 29, 2024

But then I realized that technically that can't happen because I don't think we can re-export packages that we imported, right?

You mean something like the following? If so, I believe that should already work.

library foo;

// Library foo exports all of library `geometry`.
export 'package:geometry/geometry.dart';

@matthew-carroll
Copy link

@felangel I thought I tried that and it didn't work, but maybe it does. Maybe I got something wrong in the syntax.

Doing this with many packages might benefit from a pubspec level re-export. But if that export works as described, then I assume that would at least be enough to get around the concerns from @escamoteur right?

@felangel
Copy link

@felangel I thought I tried that and it didn't work, but maybe it does. Maybe I got something wrong in the syntax.

Doing this with many packages might benefit from a pubspec level re-export. But if that export works as described, then I assume that would at least be enough to get around the concerns from @escamoteur right?

Yeah afaik that should already work and yeah flutter could just re-export Listenable, ChangeNotifier, etc from the underlying dart package. Flutter already does this with package:meta.

library foundation;

export 'package:observable/observable.dart' show
  Listenable,
  ChangeNotifier,
  ValueListenable,
  ValueNotifier;

@lrhn
Copy link
Member

lrhn commented May 29, 2024

I can see a problem here. If I were to make a new package:observable, I likely wouldn't use the current Flutter API, which means it would be breaking for flutter to switch to using it anyway. (For example, I'd definitely use a subscription object with a cancel, instead of having a removeListener. And then Listeneable is a one-method interface, and I'd probably not even have it.

Moving the code into a dart: platform library seems unnecessary. It's in a package today, just a package that's tied to the Flutter SDK.
So we can do something, but I'm not sure it's what you'd want. It'd be better to make Flutter extract their implementation into a more reusable package. It'll probably be version-locked in the Flutter SDK, but for everybody else, it's just a package.

@jhionan
Copy link

jhionan commented May 29, 2024

Upvote on extracting observable / geometry packages, and keep it in the hands of dart team or flutter team.

But I'm against adding it to Dart SDK

@caseycrogers
Copy link

caseycrogers commented May 29, 2024

A few thoughts:

A. It looks like bundling into one issue both the listening method changes and pulling the API out of Flutter so it can be used in Dart contexts has confused the conversation. Maybe we should have a separate issue for the listening method changes and edit the title and original issue explaining that that part of the conversation has been moved elsewhere?

B. Being able to use The Listenable/Notifier API in pure Dart would be super helpful. For me, I like using the API for my core state management primitives. Many state management packages like Riverpod have their own equivalents to ValueNotifier (eg NotifierBase in Riverpod).
Two problems with using the Listenable/Notifier API directly for SM in your app, or for implementing/extending them in your public SM package:

  1. Developers often want to run pure dart unit tests on business logic in their SM system. You can't do this if you're depending on Listenable-you have to spin up a widget test that runs the Flutter engine even if you're not testing anything that actually uses the Flutter engine.
  2. Developers might want to depend on parts of their business logic from the server or from a non-flutter frontend environment (<-- this one I'm not personally familiar with but have heard of, maybe someone else can add specific examples?)
    I think it'd be especially helpful if SM packages could implement Listenable instead of creating their own bespoke system. One big advantage is it'd allow for more separation of concerns and interoperability between SM packages because they could all expose public APIs that work with a common interface rather than each implementing their own bespoke listenable API.

@kevmoo
Copy link
Member

kevmoo commented May 29, 2024

Agree @caseycrogers – this issue is PURELY about moving EXISTING Flutter types somewhere they can be shared.

We DO NOT want to confuse that with (very valid) discussions about using Stream, etc.

@kevmoo
Copy link
Member

kevmoo commented Jun 1, 2024

Sorry for the churn here. Closing this request out in favor of flutter/flutter#149466

The mentioned types in this issue in particular would likely be moved to a (new) package, not the Dart SDK. Instead of renaming everything, we'll start with the design issue and open up implementation issues if/when we decide to move forward.

Thanks for the inspiration @escamoteur

@kevmoo kevmoo closed this as completed Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants