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

initial call location counting for Vars #60

Closed bbarker closed 2 years ago

bbarker commented 7 years ago

This seems to work ... except in one case. For some reason, the doge example looks as though it doesn't need allowMultiMutations but I had to set val count: Var[Int] = Var(0, allowMultiMutations = true) for it to work.

Currently the option is not configurable on the JVM; non-production is assumed in that case.

bbarker commented 7 years ago

I like the idea of having multiple apply methods, but I would argue slightly in favor of having apply be the safer option by default. Although they might be surprised the first time they see an error, that surprise will be quickly overcome, whereas the surprise from mutating in multiple locations might be worse and be less easy to overcome. "Safe by default, extra syntax when wanting to be unsafe" seems like a generally good principle to me.

bbarker commented 7 years ago

Since this code will be optimized away in production, I could even have the error message direct them to the correct apply method should they want to use multiple mutations. Maybe 'Var.unlimited`.

OlivierBlanvillain commented 7 years ago

Although they might be surprised the first time they see an error, that surprise will be quickly overcome, whereas the surprise from mutating in multiple locations might be worse and be less easy to overcome.

Then it must definitely be an warning instead of an exception, runtime errors are just too unfriendly in my opinion.

The other point that make me make slightly hesitant is that I don't want to be too opinionated without having a super clean/simpler alternative to propose. Maybe after hacking some more on the TodoMVC we realise that a single multi location mutated Var for all events is actually a reasonable pattern...

OlivierBlanvillain commented 7 years ago

Changes look fine to me! I think I would still prefer a warning instead of an exception, as runtime exceptions make for a very bad first time interaction with a library.

Also I propose that we give the imitate construct a shot before deprecating multi spot mutations on a var :)