danschultz / frappe

A Dart package for functional reactive programming
http://pub.dartlang.org/packages/frappe
MIT License
57 stars 13 forks source link

make it easier to extend / mixin Property & EventStream #40

Open Andersmholmgren opened 9 years ago

Andersmholmgren commented 9 years ago

I want to find a really clean way to integrate frappe with polymer. I want a class that is

So far I've sketched out the following

class ObservableProperty<T> extends PropertyProxy<T> with ChangeNotifier {
  StreamSubscription _subscription;
  T _oldValue;
  final Symbol _propertyName;

  ObservableProperty(Property<T> property, this._propertyName) : super(property);

  @override
  void observed() {
    if (_subscription == null) {
      _subscription = listen((T v) {
        final old = _oldValue;
        _oldValue = v;
        notifyPropertyChange(_propertyName, old, v);
      });
    }
  }

  @override
  void unobserved() {
    if (_subscription != null) {
      _subscription.cancel();
    }
  }
}

Unfortunately PropertyProxy is like

class PropertyProxy<T> implements Property<T> {
  final Property _property;
  PropertyProxy(this._property);

  @override
  Property operator *(Property other) => _property * other;

 ... 74 other methods like this :-(

That is because there is no easy way to extend Property or mix it in. Firstly all the constructors are factories (which I think is just a left over from previous versions of Frappe) but also the methods directly invoke property constructors like

new Property.fromStream(

so will not return any derived class. Totally understandable but a pain when trying to extend.

Please add some support for making this easier.

Of course I wouldn't be upset if you directly supported the observe package so I didn't need to build the extension ;-)

danschultz commented 9 years ago

Here's what I'm thinking:

class Property<T> extends Reactable<T> {
  Property._(Stream<T> stream, bool hasInitialValue, [T initialValue]) {
    ...
  }

  Property(Stream<T> stream) => this._(stream, false);
  Property.withInitialValue(T initialValue, Stream<T> stream) => this._(stream, true, initialValue);
}

The factory constructors for Property.fromStream() and Property.fromStreamWithInitialValue() will be deprecated.

Andersmholmgren commented 9 years ago

That sounds great. The other issue though is methods like

Property asyncExpand(Stream convert(T event)) => new Property.fromStream(super.asyncExpand(convert));

That is always going to return a Property not a subtype there of. You've already hit the problem with Property and EventStream doing similar things for their method implementations but returning different types.

One option here is to change things like

abstract class Reactable<T, R extends Reactable> extends Stream<T> {
  R asyncExpand(Stream convert(T event)) =>
    createFromStream(super.asyncExpand(convert));

  R createFromStream(Stream stream);
}

class Property<T, P extends Property> extends Reactable<T, P> {
  P createFromStream(Stream stream) =>
    new Property.fromStream(stream);
}

I do this in places like shelf_route to support sub classing the Router.

An upside for you is that you could probably remove much of the code in Property and EventStream as you could move the implementations into Reactable. The downside is that Dart has no protected members so your API is polluted with methods like createFromStream.

In shelf_route I tackle that by separating the interface of a Router from the implementation RouterImpl. Only RouterImpl has methods like createFromStream.

Another alternative would be to use an Abstract Factory Pattern approach and create a factory like

abstract class ReactableFactory<R extends Reactable> {
  R createFromStream(Stream stream);   
}

and then take that in the Reactable contstructor. Still part of your public api though

danschultz commented 9 years ago

Adding a generic for the return type was actually the first path I went down :) But, I ran into an issue with it, can't remember what though.

I agree, having sub-classes handle the returns is not ideal, so let me take another stab at it to see if it's doable. I'll create a ticket.

danschultz commented 9 years ago

I created ticket #41 for tracking the move of implementation methods to Reactable.

danschultz commented 9 years ago

I created ticket #42 for adding public non-factory constructors to Property.

Andersmholmgren commented 9 years ago

haha yeah it can be painful at times. Particularly it can be painful create builders with generics. At least in Java it is