Monix port #130

mariusmuja · 2017-12-13T21:38:56Z

This is a port of Outwatch to the Monix library.

mariusmuja · 2017-12-13T21:51:57Z

src/test/scala/outwatch/ScenarioTestSpec.scala

+      }
+      _ <- Task {
+        list.childElementCount shouldBe 1
+      }.executeWithFork


This is because Monix uses by default a batched execution model, so we need to "fork" the assertion to the end of the javascript execution queue, otherwise it might execute before all the effects of the event dispatch execute.

fdietze · 2017-12-14T09:13:04Z

Would it be worth it to port directly to a preliminary Monix 3 Release?

mariusmuja · 2017-12-14T16:36:48Z

All that's needed to upgrade to Monix 3 is to update the dependency version, there seem to be no breaking changes in the API used.

fdietze · 2017-12-14T18:17:56Z

Interesting. Are monix.Task and cats.effect.IO compatible?

LukaJCB · 2017-12-14T18:29:08Z

They are!

fdietze · 2017-12-15T16:28:16Z

src/main/scala/outwatch/dom/helpers/DomUtils.scala

-    attributeReceivers.combineLatest(childStreamReceivers)
+    attributeReceivers.startWith(Seq(Seq.empty)).combineLatest(
+      childStreamReceivers.startWith(Seq(Seq.empty))
+    ).dropWhile { case (a, c) => a.isEmpty && c.isEmpty }


What is this dropWhile good for in this case?

Without doing startwith(Seq(Seq.empty)) and dropWhile, the combinedLatest would emit an element only when both the attributeReceivers and childStreamReceivers emit. This makes it emit when either of them emits.

So the behavior of combineLatest is different in monix?

That's what I thought too, but from reading the documentation now, it seems that combineLatest in rxjs should behave the same as in Monix. Let me look into why I had to do this (or why it's not needed with rxjs/rxscala).

The difference is between the Observable.combineLatest rxscala companion object method and the Monix Observable.combineLatestList. The Monix combineLatestList doesn't emit an empty list if the argument list is empty, rxscala does.

Should we have a test for this?

Now the code doesn't do .startWith(Seq(Seq.empty))....dropWhile { case (a, c) => a.isEmpty && c.isEmpty } any more, instead it emits an Observable(Seq.empty[Attribute]) if the attributeReceivers list is empty (just like in the rxscala version). There are several tests that fail if we don't do that.

fdietze · 2017-12-15T16:32:55Z

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

+
+import scala.concurrent.Future
+
+

 final case class Store[State, Action](initialState: State,


STRef is only used in Store. Is it possible to remove it entirely?

STRef is used in cases where a global variable with a referential transparent API is wanted (such as the store). It's possible to remove it, but without it the store would need to be passed as an argument to all components that use it.

fdietze · 2017-12-15T16:37:56Z

src/main/scala/outwatch/http/Http.scala

+import scala.scalajs.js
+import scala.scalajs.js.typedarray.ArrayBuffer
+import scala.scalajs.js.|
+

 object Http {


It seems like here is more going on than a simple port. Could you explain what you did here? I'm also not using the Http module myself, so this should be reviewed by someone who does.

If its too different it should be a separate commit.

rxscala has an Observable.ajax method. Monix doesn't have that, so essentially I had to re-implement that functionality (based on org.scalajs.dom.ext.Ajax).

fdietze · 2017-12-15T16:41:00Z

src/test/scala/outwatch/DomEventSpec.scala

@@ -418,8 +419,11 @@ class DomEventSpec extends JSDomSpec {

    document.getElementById("input").dispatchEvent(inputEvt)

-    winClicked shouldBe true
-    docClicked shouldBe true
+    Task {


Could you please explain, why this is neccessary here?

This is because Monix uses by default a batched execution model, so we need to "fork" the assertion to the end of the javascript execution queue, otherwise it might execute before all the effects of the event dispatch execute.

fdietze · 2017-12-15T16:44:35Z

src/main/scala/outwatch/Sink.scala

-import rxscalajs.facade.SubjectFacade
-import rxscalajs.subscription.Subscription
-import rxscalajs.{Observable, Observer, Subject}
+import monix.execution.Scheduler.Implicits.global


Is it safe to import the global execution scheduler everywhere?

It is ok, but it makes the batched execution model the default everywhere. It would be better to have the scheduler an implicit argument where it's needed, but that would have been a PR with more changes vs the rxscala version (which essentially doesn't have an option of picking a different Scheduler).

fdietze · 2017-12-15T16:46:55Z

src/test/scala/outwatch/ScenarioTestSpec.scala

-      document.getElementById("plus").dispatchEvent(event)
-      document.getElementById("counter").innerHTML shouldBe i.toString
-    }
+    Task {


This whole block looks complicated compared to the old one. Is there a simpler way to write it?

The simpler way would be to have the scheduler injected and use one with a synchronous execution model. Then the test code would be identical as before.

Actually, the simplest way would be to have the synchronous scheduler everywhere (then it would be the same execution model as rxjs and the test could have stayed the same). But I don't like that, the batched execution model is a better choice to keep the page responsive.

does the test fail with the batched execution model?

Actually, it was not the batched execution model that was causing the async boundary here, it was the Observable.merge that uses a buffered subscriber to properly implement the back-pressure protocol. I've replaced it with a simple mergeSync, so the test stays unchanged.

fdietze · 2017-12-15T16:57:46Z

@mariusmuja do you know how the fullOpt build size is affected?

mariusmuja · 2017-12-15T17:32:19Z

@mariusmuja do you know how the fullOpt build size is affected?

Because of the dead code elimination that Scala.js does, it's difficult to say precisely. But assuming that no other features of Monix are used in addition to what the library (and the tests) are using, the bundled size seems to be smaller. I did a comparison of the test bundle size a while ago and posted it in #96 , here it is:

With RxJS:

-rw-rw-r--   1 marius marius  313987 Nov 21 21:41 outwatch-test-opt-bundle.js.gz
-rw-rw-r--   1 marius marius 1593988 Nov 21 21:34 outwatch-test-opt-bundle.js
-rw-rw-r--   1 marius marius  837289 Nov 21 21:33 outwatch-test-opt.js

Monix port:

-rw-rw-r--   1 marius marius  240514 Nov 21 21:38 outwatch-test-opt-bundle.js.gz
-rw-rw-r--   1 marius marius 1077036 Nov 21 21:38 outwatch-test-opt-bundle.js
-rw-rw-r--   1 marius marius 1046342 Nov 21 21:37 outwatch-test-opt.js

fdietze · 2017-12-15T21:07:01Z

I'm also a bit concerned that the users now have to deal with both Monix Tasks and IO. You said that they are compatible? But could outwatch be used with one of them only? How about IO and the batched execution context?

mariusmuja · 2017-12-15T23:50:10Z

I'm also a bit concerned that the users now have to deal with both Monix Tasks and IO. You said that they are compatible? But could outwatch be used with one of them only? How about IO and the batched execution context?

Users don't have to deal with both. For example, Task is no longer used in the outwatch code (and before it was only used in the tests).
IO doesn't know or care about the batched execution context, it's only Monix that does.

mariusmuja · 2017-12-16T03:42:06Z

The Scheduler is now injected as an implicit argument where needed. For example, this allows a TrampolineScheduler to be injected in the two tests where we want synchronous behaviour for easier testing.

Client code will require an implicit Scheduler in scope, but users will get a compile error asking to import monix.execution.Scheduler.Implicits.global (similar to the requirement to have an ExecutionContext in scope when working with futures... in fact a Scheduler is an ExecutionContext).

…olinedScheduler in tests where we want synchronous behaviour.

fdietze

I finished porting my project to this branch, and so far its working very well. Thank you for this PR!

LukaJCB · 2017-12-19T18:26:10Z

src/main/scala/outwatch/Pipe.scala

@@ -16,16 +18,16 @@ object Pipe {
    * @tparam T the type parameter of the elements
    * @return the newly created Pipe.
    */
-  def create[T](seeds: T*): IO[Pipe[T, T]] = create[T].map { pipe =>
+  def create[T](seeds: T*)(implicit s: Scheduler): IO[Pipe[T, T]] = create[T].map { pipe =>


I wonder, since we now have both IO and Task in our project as dependencies, if we should continue using one or the other, or abstract over them with the type classes found in cats-effect, i.e.

def create[F[_]: Sync, T](seeds: T*): F[Pipe[T, T]]

What do you guys think? :)

On an unrelated note, I also think we could get rid of the varargs here, I don't think it really ever makes sense to prepend items to a stream?

I'm in favor of abstracting over IO and Task, this would also allow using outwatch without IO Monads again.

One seed item is definitely enough for state-management, which is actually what we need for reactive UIs ;) (Make Outwatch FRP-backend agnostic #96)

@LukaJCB could you please open PRs/Issues for this? I'll merge now.

+1 on abstracting over IO and Task.

I created #135 and #136 :)

LukaJCB

Left a small note, wanting to hear your opinion, overall I'm super happy with this, it looks really great!

LukaJCB · 2017-12-19T19:54:38Z

src/main/scala/outwatch/Sink.scala

  }

-  private[outwatch] final case class SubjectSink[T]() extends Subject[T](new SubjectFacade) with Sink[T] {
-    override private[outwatch] def observer = this
+  private[outwatch] final case class SubjectSink[T]()(implicit scheduler: Scheduler) extends Observable[T] with Sink[T] {


Why exactly do we need the scheduler here? Could we not add it to the methods? This would unclutter our code a bit, no?

In fact, why don't we do:

def observer: Observer[T]? So we wouldn't need the scheduler at all?

We need it to create the Subscriber inside the SubjectSink (the Sink.observer is actually a Subscriber now) . If I would have left it an Observer, all the Sink.redirect operations would have required a Scheduler (and also many operations in PipeOps). This way they just use the Scheduler from the source Sink's Subscriber.

Yeah, that makes sense, however, I think creating a new Handler or Pipe is going to be a much much more common operation, then redirect, so I think it'd be better if the individual methods required the implicit evidence, no?

Not quite, EmitterBuilder's --> would require an implicit Scheduler in that case.

mariusmuja force-pushed the monix_port branch from 830048c to fd49a6d Compare December 13, 2017 21:42

mariusmuja commented Dec 13, 2017

View reviewed changes

mariusmuja force-pushed the monix_port branch from fd49a6d to eddc673 Compare December 14, 2017 06:59

Monix port

20a2282

mariusmuja force-pushed the monix_port branch from eddc673 to 20a2282 Compare December 14, 2017 07:06

Updated monix dependency

2048581

mariusmuja mentioned this pull request Dec 14, 2017

DOM dsl refactor #132

Merged

Fix compile warning

aa17ec9

fdietze reviewed Dec 15, 2017

View reviewed changes

Revert simple code move (makes diff easier to read)

a3f283b

Simplified tests

1776f41

mariusmuja force-pushed the monix_port branch from f11cfd0 to 1776f41 Compare December 15, 2017 23:35

Use implicitly injected Scheduler in the library and inject the Tramp…

3a29dd0

…olinedScheduler in tests where we want synchronous behaviour.

mariusmuja force-pushed the monix_port branch from dfee735 to 3a29dd0 Compare December 16, 2017 03:45

mariusmuja mentioned this pull request Dec 16, 2017

Multiple child node fix #134

Merged

fdietze approved these changes Dec 19, 2017

View reviewed changes

LukaJCB reviewed Dec 19, 2017

View reviewed changes

LukaJCB approved these changes Dec 19, 2017

View reviewed changes

fdietze merged commit b55b20d into outwatch:master Dec 19, 2017

LukaJCB reviewed Dec 19, 2017

View reviewed changes


		import scala.concurrent.Future



		final case class Store[State, Action](initialState: State,

Monix port #130

Monix port #130

Conversation

mariusmuja commented Dec 13, 2017

Choose a reason for hiding this comment

fdietze commented Dec 14, 2017

mariusmuja commented Dec 14, 2017

fdietze commented Dec 14, 2017

LukaJCB commented Dec 14, 2017

fdietze Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmuja Dec 15, 2017 • edited Loading

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

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

fdietze commented Dec 15, 2017

mariusmuja commented Dec 15, 2017

fdietze commented Dec 15, 2017

mariusmuja commented Dec 15, 2017

mariusmuja commented Dec 16, 2017

fdietze left a 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

LukaJCB left a 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

fdietze Dec 15, 2017 •

edited

Loading

mariusmuja Dec 15, 2017 •

edited

Loading