Handler/Sink refactor, added Pipe trait #91

mariusmuja · 2017-11-12T19:17:27Z

I've done some refactoring to Handler/Sink and added a new trait Pipe which is like Handler, but exposes the sink and source as different members so it's possible to transform them individually (map the source, redirect the sink,... ) while keeping the Pipe structure. I've been experimenting which this in outwatch-extras lately (https://github.com/mariusmuja/outwatch-extras/blob/io_monad_refactor_monix/outwatch-redux/src/main/scala/outwatch/redux/package.scala) and I found the concept of Pipe very useful.

A Handler[A] is now a Pipe[A,A]. A Store[S,A] is also a Pipe[A,S]. In outwatch-redux I have an Effects[E, R], which is also a Pipe[E, R].

A Pipe also removes the need for the old ObservableSink.

This PR has some overlap with #83 as it provides some of the same operations (mapSink - contramapMap, mapSource - comapMap), but it doesn't requires the types to be the same.

It also address #90 by providing transforming operations on the sink and source, but to use them one needs to import outwatch.advanced._ because they can be more easily used incorrectly and be a cause of bugs.

cornerman · 2017-11-12T19:40:44Z

src/main/scala/outwatch/dom/Handlers.scala

@@ -22,7 +19,7 @@ trait Handlers {
  def createBoolHandler(defaultValues: Boolean*) = createHandler[Boolean](defaultValues: _*)
  def createNumberHandler(defaultValues: Double*) = createHandler[Double](defaultValues: _*)

-  def createHandler[T](defaultValues: T*): IO[Handler[T]] = Sink.createHandler[T](defaultValues: _*)
+  def createHandler[T](defaultValues: T*): IO[Handler[T]] = Handler.create[T](defaultValues: _*)


I know, this was there before. but why have two methods for creating a handler? This is Handler.create and createHandler.

I added Handler.create because it think the place to create a handler should be in Handler (somewhat analogous to Sink.create, Observable.create...). I kept Handlers.createHandler for backwards compatibility, maybe it should be deprecated?

Good idea. Deprecating the old one sounds like a good idea. What about the createNumberHandler, createBoolHandler, ...? Not sure whether, they really add value or whether something like Handler.create[Boolean] is good enough.

Yes, I think something like Handler.create[Boolean] is good enough. The ones that add some value is the ones taking event types, such as createInputHandler, createMouseHandler as one doesn't need to import the respective events.

Maybe add them to Handler then? Makes them more consistent.

I think prefer Handler completely generic. Plus Handler is in the outwatch package (not in outwatch.dom package), so it shouldn't "know" about dom types.

You are right!

fdietze · 2017-11-12T22:16:26Z

Why did you add Pipe instead of giving Handler two type arguments? Why do we need both?

mariusmuja · 2017-11-12T22:24:34Z

We don't need both, I kept Handler with one type argument just for backwards compatibility (and it's a type alias for Pipe[A,A]).

Also, not sure if Pipe is the best name. Any other suggestions? Should we change change Handler to have two type arguments?

LukaJCB · 2017-11-12T22:44:42Z

src/main/scala/outwatch/package.scala

+  type >-->[-I, +O] = Pipe[I, O]
+  val >--> = Pipe
+
+  type Handler[A] = Pipe[A, A]


If we do this, we can call mapSink on a Handler and get back a Pipe[A, B] not sure if that makes sense.

Yes, I see what you mean. How do you feel about changing Handler to have two type parameters? I shouldn't break almost any client code, Handler type is not typically used much in client code.

You're right, I think the solution below is probably the best :)

fdietze · 2017-11-12T22:54:25Z

I prefer the whole thing to be as simple as possible. Therefore I suggest something like:

trait Sink[-I] { private def observer:Observer[I] }
type Handler[-I,+O] = Sink[I] with Observable[O]

In my opinion even wrapping Observer with Sink as a bit too much. It introduces new terminology with marginal gain (private observer) and makes it a bit incompatible with rxjs.

So maybe even just:

type Handler[-I,+O] = Observer[I] with Observable[O]

mariusmuja · 2017-11-12T23:12:19Z

@fdietze Having observer private in Sink guarantees correct handling of subscriptions in Sink redirects, which eliminates a whole class of possible resource leaks.

fdietze · 2017-11-12T23:38:00Z

With guarantees you are talking only about the fact that the user cannot use the internal observer, right? redirect could still be added via implicit on Observer and provide this safety. Do I understand correctly?

I'm interested in an example for such a resource leak.

mariusmuja · 2017-11-13T00:22:55Z

Yes, that's what I meant. Redirect could obviously still be added using an implicit on Observer, but it would be possible to use the observer directly and have resource leaks without properly handling the subscription(s).

fdietze · 2017-11-13T14:21:58Z

I took some time to think about the whole Sink concept. For me it looks like Sink is only there for safety, but it only provides a little safety for the subscription case. Even with Sink one can still produce resource leaks by using <--. It is also possible to produce resource leaks by re-creating components.

One possible solution:
Instead of trying to hide functionality from the user, make it clear that certain things could produce resource leaks. In many cases the unsafe cases are very useful for testing. We could use @deprecate to inform the user at compile-time with a warning:

scala> trait A { def f:Int; def g:Int} // this could be the Observer
defined trait A

// Handler, which implements the Observer-trait, but deprecates the usage of the unsafe methods
scala> trait B extends A { def f = 5; @deprecated("Using g is unsafe", since="forever") def g = 17 }
defined trait B

scala> val b = new B {}
b: B = $anon$1@1e606f64

scala> b.f
res1: Int = 5

scala> b.g
<console>:13: warning: method g in trait B is deprecated (since forever): Using g is unsafe
       b.g
         ^
res2: Int = 17

This is far from perfect. One could still do (b:A).g and miss the warning. I just wanted to present the basic idea.

I'm wondering if Monix has preventions against resource leaks. Do you know? @mariusmuja

fdietze · 2017-11-14T13:29:39Z

Independently of the Sink discussion, I strongly agree that the concepts of Hander and Pipe should be merged. Both are a Pipe[I,O] (or how we name it).

Proposal:

type Pipe[-I,+O] = Sink[I] with Observable[O]
type Handler[T] = Pipe[T,T]

Pipe can implement:

Profunctor

Handler can implement:

Functor (map returns Observable)
Contravariant (contramap returns Sink)
Invariant (imap returns Handler)

(I'm not exactly sure about the Handler type classes)

All the other methods are added via implicits, or we use a trait instead (which one is better?):

trait Pipe[-I,+O] extends Sink[I] with Observable[O]

LukaJCB · 2017-11-14T13:49:34Z

I agree with above except for a couple of points:

Handler can only implement Invariant, map and contramap should just be syntax sugar, since valid Functors and Contravariant functors, should preserve structure :)

mariusmuja · 2017-11-14T20:26:41Z

I pushed some refactorings as discussed above.

It renames Pipe to Handler (changing Handler to have two type arguments), leaves Handler to be both an Observable and a Sink and extracts handler operations into a trait HandlerOps:

type Handler[-I, +O] = Observable[O] with Sink[I] with HandlerOps[I, O]

Also, Store has an implicit conversion to Handler instead of separate implicit conversions to Observable and Sink.

There's a test that randomly fails (DomEventSpec.scala:41), I cannot reproduce the failure on my machine. The test assertion seems to happen before the DOM update takes place. Jsdom doesn't implement mutation observers, any idea how to properly fix it?

mariusmuja · 2017-11-14T20:57:32Z

@fdietze My understanding is that the Observable in Monix has memory safe operations, but one can still have memory leaks by subscribing directly with an observer and not managing the subscriptions properly. Sink hides the observer to discourage this, but as you mentioned it's still possible to do so using <---.

fdietze · 2017-11-15T12:08:40Z

src/main/scala/outwatch/util/Store.scala

@@ -12,7 +12,7 @@ import scala.language.implicitConversions

 final case class Store[State, Action](initialState: State,
                                           reducer: (State, Action) => (State, Option[IO[Action]]),
-                                           handler: Observable[Action] with Sink[Action]) {
+                                           handler: Handler[Action, Action]) {


Why does Store contain a Handler instead of being a Handler?

A Handler is a Sink (Observable with Sink) and Sink is a sealed trait, so nothing can extend Handler. It's one of the reasons I initially had the Handler wrap the Observable and Sink.

fdietze · 2017-11-15T12:15:18Z

src/main/scala/outwatch/dom/Handlers.scala

+  def createHandler[T](defaultValues: T*): IO[Handler[T, T]] = Handler.create[T](defaultValues: _*)
+
+
+  implicit class HandlerCreateHelpers(handler: Handler.type) {


Why is this not directly in the Handler object in Handler.scala?

I preferred to keep Handler completely generic. Also, Handler is in the outwatch package (not in outwatch.dom package), so it shouldn't "know" about dom types.

fdietze · 2017-11-15T12:17:18Z

src/main/scala/outwatch/dom/package.scala

@@ -10,6 +10,15 @@ package object dom extends Attributes with Tags with Handlers {
  type VNode = IO[VNode_]
  type VDomModifier = IO[VDomModifier_]

+  type Observable[+A] = rxscalajs.Observable[A]


I really like the direction of outwatch becoming frp-backend agnostic. 👍

fdietze · 2017-11-15T12:24:35Z

src/main/scala/outwatch/advanced/package.scala

+
+import rxscalajs.Observable
+
+package object advanced {


I don't understand why we need an advanced package, or how these methods could be unsafe. I find these methods especially useful to write correct, bug-free code.

Do you have an example of an unsafe usage?

It's safe as long the original Observable is transformed, but a different Observable could be returned.

For example, let's say one would like to emit two elements every time a button is clicked and writes this (incorrect) code:

Handler.create[Int] { handler => val many = handler.transformSink[MouseEvent](_ => Observable.of(1,2)) button(click --> many) }

However, this doesn't work as expected because the handler's observable is closed immediately on creation and the click handler doesn't do anything.

How is that unsafe? The whole observable is replaced by another one. We cannot protect the user to use the wrong operators. Am I missing something?

Safe was not the correct term to use. What I was trying to say is that in the above example the button click doesn't do anything because the observable is completed after emitting (1, 2) , which is probably not what is intended, instead it should have been:

Handler.create[Int] { handler => val many = handler.transformSink[MouseEvent](_.concatMap(Observable.of(1,2))) button(click --> many) }

Often the transform... methods will require the use of higher order observables. There's nothing wrong with that, but they are easier to use incorrectly, which is why I placed them under the outwatch.advanced._ import. It also aligns with one of the Outwatch goal: "Removing or restricting the need for Higher Order Observables".

But aren't we already importing higher-order Observables from https://lukajcb.github.io/rxscala-js/latest/api/rxscalajs/Observable.html?

@LukaJCB ?

I've actually changed my mind. We should place the transform... methods in the HandlerOps trait. They are no more "advanced" than Sink.redirect, which are available to the user.

fdietze · 2017-11-15T12:28:17Z

Interesting API difference between Monix and rxjs:
https://monix.io/api/2.3/monix/reactive/subjects/Subject.html (two type parameters)
https://lukajcb.github.io/rxscala-js/latest/api/rxscalajs/Subject.html (one type parameter)

fdietze · 2017-11-15T14:58:23Z

While looking at the code, it seems like this could be done in a simpler way. I'll give it a try soon.

fdietze · 2017-11-15T19:39:20Z

@mariusmuja You had good answers to my questions, so I'm fine with the code now.

Last point: @LukaJCB mentioned that having Handler with two type parameters gives problems with partial unification: https://gist.github.com/djspiewak/7a81a395c461fd3a09a6941d4cd040f2

@LukaJCB, what do you suggest?

Would it help to go back to only one type-parameter for Handler? But then we have the same problems with Pipe.

type Pipe[-I,+O] = Sink[I] with Observable[O]
type Handler[T] = Pipe[T,T]

vs

type Handler[-I,+O] = Sink[I] with Observable[O]

LukaJCB · 2017-11-20T15:14:44Z

I think most people will just use Handler so keeping it with one param seems like a good idea. If power users want to use Pipe they can use partial-unification.

fdietze · 2017-11-20T17:37:05Z

src/main/scala/outwatch/Pipe.scala

+
+trait PipeOps[-I, +O] { self : Pipe[I, O]  =>
+
+  def mapSink[I2](f: I2 => I): Pipe[I2, O] = Pipe(redirectMap(f), self)


Sadly, with all these methods we cannot overload and return a Handler if I and O are the same type...

@fdietze What do you think about having this:

implicit class HandlerOps[T](self: Pipe[T, T]) { def imap[S](read: T => S)(write: S => T): Handler[S] = self.mapPipe(write)(read) def lens[S](seed: T)(read: T => S)(write: (T, S) => T): Handler[S] = { val redirected = self.redirect[S](_.withLatestFrom(self.startWith(seed)).map { case (a,b) => write(b,a) }) Handler(redirected, self.map(read)) } def transformHandler[S](read: Observable[T] => Observable[S])(write: Observable[S] => Observable[T]): Handler[S] = self.transformPipe(write)(read) }

and leaving type Handler[T] = Pipe[T, T]. This way a Handler is a Pipe, transformHandlerSink, and transformHandlerSource will not be needed any more (will be the same as transformSink and transformSource).

Good idea, I'll give it a try.

Done. I think it's much cleaner now.

fdietze · 2017-11-21T15:29:40Z

I'll merge when there's one more review.

mariusmuja · 2017-11-21T19:48:57Z

src/main/scala/outwatch/package.scala

+
+  implicit class HandlerOps[T](val self: Handler[T]) extends AnyVal {
+
+    def imap[S](read: T => S)(write: S => T): Handler[S] = self.mapPipe(write)(read)


We should have the sink/source transformation order consistent between Pipe and Handler. Either transform the sink first everywhere, or the source first everywhere. Intuitively for me was to have the sink first, because something sent into the sink gets emitted by the observable afterwards.

Good catch! Yes this should be consistent. My motivation for this order was that you can easily rewrite a handler.map(f) to handler.imap(f)(g) by just appending the second function.

Interestingly

mapPipe is consistent with Profunctor.dimap:
https://github.com/typelevel/cats/blob/0997f43ab68aac1ea83ad0dbc0819d2ad3e5ef22/core/src/main/scala/cats/arrow/Profunctor.scala#L29

And imap with Invariant.imap:
https://github.com/typelevel/cats/blob/8d7b39407b282781c06ca580479b6cc125bcb312/core/src/main/scala/cats/Invariant.scala#L9

😅

That's interesting. In that case, let's leave them as they are.

Great. Merge?

Handler/Sink refactor, added Pipe trait.

6c1de88

cornerman reviewed Nov 12, 2017

View reviewed changes

Deprecate Handlers.create...Handler.

ddc89b9

LukaJCB reviewed Nov 12, 2017

View reviewed changes

Marius Muja added 3 commits November 14, 2017 11:10

Refactorings

7c58cb4

Ups, forgot to add file

c7bbad7

Mix in HandlerOps

2392bec

fdietze reviewed Nov 15, 2017

View reviewed changes

Move transform... methods to HandlerOps trait

565b318

fdietze added 3 commits November 20, 2017 16:41

Handler with only one Type parameter

6cb14cb

Rename PipeOps

5e1ba29

Some Pipe/Handler renames, reorder PipeOps

4846f0f

fdietze requested a review from LukaJCB November 20, 2017 16:01

fdietze approved these changes Nov 20, 2017

View reviewed changes

HandlerOps with lens and imap

d21fceb

fdietze mentioned this pull request Nov 20, 2017

Handler comap, contramap, imap and lens #83

Closed

fdietze requested a review from cornerman November 20, 2017 17:31

fdietze reviewed Nov 20, 2017

View reviewed changes

PipeOps and HandlerOps as implicit classes

669583c

Minor changes

279704b

mariusmuja commented Nov 21, 2017

View reviewed changes

mariusmuja merged commit a68965b into outwatch:master Nov 21, 2017

fdietze mentioned this pull request Nov 21, 2017

Import Sink and Observable by importing outwatch.dom._ #82

Closed

mariusmuja deleted the handler_refactor branch November 21, 2017 23:24

mariusmuja pushed a commit to mariusmuja/outwatch that referenced this pull request Nov 22, 2017

Fix backwards compatibility broken in outwatch#91

5387df2

mariusmuja mentioned this pull request Nov 22, 2017

Fix backwards compatibility #97

Merged

LukaJCB pushed a commit that referenced this pull request Nov 22, 2017

Fix backwards compatibility broken in #91

84b1037

		def createHandler[T](defaultValues: T): IO[Handler[T, T]] = Handler.create[T](defaultValues: _)


		implicit class HandlerCreateHelpers(handler: Handler.type) {


		trait PipeOps[-I, +O] { self : Pipe[I, O] =>

		def mapSink[I2](f: I2 => I): Pipe[I2, O] = Pipe(redirectMap(f), self)


		implicit class HandlerOps[T](val self: Handler[T]) extends AnyVal {

		def imap[S](read: T => S)(write: S => T): Handler[S] = self.mapPipe(write)(read)

Handler/Sink refactor, added Pipe trait #91

Handler/Sink refactor, added Pipe trait #91

Conversation

mariusmuja commented Nov 12, 2017

Choose a reason for hiding this comment

mariusmuja Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdietze commented Nov 12, 2017

mariusmuja commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdietze commented Nov 12, 2017

mariusmuja commented Nov 12, 2017

fdietze commented Nov 12, 2017

mariusmuja commented Nov 13, 2017

fdietze commented Nov 13, 2017

fdietze commented Nov 14, 2017 • edited

LukaJCB commented Nov 14, 2017

mariusmuja commented Nov 14, 2017

mariusmuja commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmuja Nov 15, 2017 • edited

Choose a reason for hiding this comment

fdietze commented Nov 15, 2017

fdietze commented Nov 15, 2017

fdietze commented Nov 15, 2017

LukaJCB commented Nov 20, 2017

Choose a reason for hiding this comment

mariusmuja Nov 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdietze commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmuja Nov 12, 2017 •

edited

fdietze commented Nov 14, 2017 •

edited

mariusmuja Nov 15, 2017 •

edited

mariusmuja Nov 20, 2017 •

edited