OlivierBlanvillain / monadic-html

Tiny DOM binding library for Scala.js
https://olivierblanvillain.github.io/monadic-html/examples/
MIT License
225 stars 24 forks source link

[WIP] tentative monoid design: scala.js crashing? #76

Closed bbarker closed 6 years ago

bbarker commented 6 years ago

This would be easy if we were ok with saying def empty: Rx[A] = Rx(null.asInstanceOf[A]), but this seems like a bad idea in a public API (I already ran into problems trying to use it in the TodoMVC example).

So, I tried another route. I try using ClassTags (no TypeTags in Scala.js) with support for some common collections, but really it would be good if we could somehow abstract for the case where A is itself a Monoid[_], and then use def empty = Rx(A.empty) (simplification), but I wasn't clever enough to figure out how to do that this afternoon. Also, this seems to crash the Scala.js compiler...

bbarker commented 6 years ago

Actually, after getting some sleep, I can probably use the current ClassTag approach to check if it is a Monoid[_] ... hopefully that won't also cause Scala.js to crash.

OlivierBlanvillain commented 6 years ago

Is your intention to have Rx.empty[List[Int]] = Rx(Nil)? This wouldn't make Rx a monoid given than e |+| Rx.empty is different from e:

e              =>  List(1, 2)  List(3)
Rx(Nil)        =>  Nil
e |+| Rx(Nil)  =>  Nil  List(1, 2)  List(3)
bbarker commented 6 years ago

Ah, I forgot List worked in that way. Yes, that was my intention. Do you think we should continue to look for a workaround or just treat Rx.empty as Rx(null) and deal with the implications in the user's code?

OlivierBlanvillain commented 6 years ago

Neither, I think Rx should not be a monoid. IMO null should never even be considered for something user facing, but even if we wanted to make Rx[Option[T]] a monoid with Rx.empty[T] = Rx(None) that wouldn't work, given that e |+| Rx.empty would not be equal to e...

bbarker commented 6 years ago

I see, you've convinced me. Then I suppose we should not use Monoid.combineAll as suggested in #72 and just go ahead and work on merging #72?

OlivierBlanvillain commented 6 years ago

It looks like there is also something for semigoups: Semigroup.combineAllOption. Sorry if I pointed you to the wrong direction :confused:

bbarker commented 6 years ago

No worries, thanks - at least I learned some things, will check out Semigroup.combineAllOption soon -it sounds like it fits the bill.