Feature request: simplify building of bidirectional routing to avoid Url -> Page -> Url cycle (especially for fragment-based)

[ivan-klass]

Rationale: Router can be used in order for SPA user to share / bookmark URLs that correspond to specific page (app state), i.e. users should be able to just copy the browser url and get a reproducible result if it's opened somewhere else (new tab / browser / device). Also, users may manually edit the URL and we want the app to adjust the route.

Issues A plain implementation like the following introduces a infinite cycle (Transaction max depth error in console) on any url change. That means, even if pushed state is the same, it's not ignored by the underlying observers, continuing the loop.

• App observes router.currentPageSignal to react either on initial url, or manual fragment part changes (browser doesn't reload the page in that case).
• Internal (app-triggered) change of the page uses router.pushState (or replaceState, if we ignore back/forward history navigation)

Workarounds I end up with a wrapper adding external / internal change flag to the Page, only producing "external" when decoded or deserialized.

import com.raquo.airstream.core.EventStream
import com.raquo.airstream.core.Observer
import com.raquo.airstream.core.Signal
import com.raquo.waypoint.Router
import io.circe.Decoder
import io.circe.Encoder

/** .internalChange - url was changed by the app script itself (using history API) */
case class Bidirectional[+Page] private(private val internalChange: Boolean, private val page: Page)

object Bidirectional:

  def serialize[Page](using enc: Encoder[Page])(rPage: Bidirectional[Page]): String = enc(rPage.page).noSpaces
  def toRoute[Page, RouteArgs](enc: Page => RouteArgs)(rPage: Bidirectional[Page]): RouteArgs = enc(rPage.page)

  // the actual decoding only happens when it was an external url change
  def fromRoute[RouteArgs, Page](d: RouteArgs => Page): RouteArgs => Bidirectional[Page] =
    d.andThen(p => Bidirectional(internalChange = false, p))

  def deserializeOr[Page](default: Page)(using dec: Decoder[Page])(pageStr: String): Bidirectional[Page] =
    Bidirectional(
      internalChange = false,
      io.circe.parser
        .parse(pageStr)
        .flatMap(dec.decodeJson)
        .getOrElse(default)
    )

  case class BoundTo[Page] private[Bidirectional] (externalChanges: Signal[Page], inAppUpdates: Observer[Page])

  def bindTo[Page](router: Router[Bidirectional[Page]]): BoundTo[Page] =
    val pageSignal = router.currentPageSignal
    BoundTo(
      // don't propagate internal changes
      pageSignal.changes
        .collect { case Bidirectional(false, externalPageChange) => externalPageChange }
        .toSignal(pageSignal.now().page),
      Observer[Page](nextPage => router.pushState(Bidirectional(internalChange = true, nextPage)))
    )

so then I can use it like

import com.raquo.laminar.api.L.*
import com.raquo.waypoint.*
type Page = String

val route = Route[Bidirectional[Page], String](
  encode = Bidirectional.toRoute(identity),
  decode = Bidirectional.fromRoute(identity),
  pattern = root / segment[String] / endOfSegments,
  basePath = Route.fragmentBasePath
)

val router = new Router[Bidirectional[Page]](
  routes = List(route),
  getPageTitle = _ => "My Page",
  serializePage = Bidirectional.serialize,
  deserializePage = Bidirectional.deserializeOr("")
)(popStateEvents = windowEvents(_.onPopState), owner = unsafeWindowOwner)

val page = Bidirectional.bindTo[Page](router)

def main(): Unit =
  renderOnDomContentLoaded(
    org.scalajs.dom.document.querySelector("#appContainer"),
    input(
      // app page --> url
      onInput.mapToValue --> page.inAppUpdates,  // naive example, better have debounce
      // url --> app page
      value <-- page.externalChanges
    )
  )

It would be nice if something serving the same purpose will be available out of the box.

I would be happy to help with PR in case some initial guidance were provided.

If I just missing something, I can help with adding a note to the documentation.

[raquo]

Hi, you mentioned "A plain implementation like the following introduces a infinite cycle" – but it doesn't seem that you've included the code for it in the issue? If I understand correctly, the code you shared is that of your workaround, but I would like to see the code that's actually failing with the infinite loop.

OTOH even without the workaround, I don't yet see why a combination of onInput --> pushState and value <-- currentPageSignal would cause an infinite loop, as value does not feed into onInput, so I need to see your plain code that has that loop.

In general, yes, calling pushState when observing currentPageSignal would cause an infinite loop if there are no filters in the loop – but, that isn't any different than e.g. updating a Var from which a signal is derived, in the observer of that signal – that would also cause an infinite loop, because that's what the code is instructed to do. Is there a router-specific use case that needs to pushState in the observer of currentPageSignal?

[raquo]

To clarify what I'm asking, I see your workaround code, but I don't readily understand how to make that code fail with an infinite loop by removing the workaround. So I'd like to see the plain code that tries to do the same thing as your code that uses the workaround, but fails, due to the infinite loop issue that the workaround fixes.

[ivan-klass]

@raquo sure thing! Here's an example

type Page = Option[String]

import com.raquo.laminar.api.L.*
import com.raquo.laminar.api.L.given
import com.raquo.waypoint.*
import io.circe.syntax.given

val route = Route.onlyQuery[Page, Option[String]](
  encode = identity,
  decode = identity,
  pattern = (root / endOfSegments) ? param[String]("q").?,
  basePath = Route.fragmentBasePath
)

val router = new Router[Page](
  routes = List(route),
  getPageTitle = _ => "My Page",
  serializePage = _.asJson.noSpaces,
  deserializePage = pageStr =>
    io.circe.parser
      .parse(pageStr)
      .flatMap(_.as[Page])
      .toOption
      .flatten
)(popStateEvents = windowEvents(_.onPopState), owner = unsafeWindowOwner)

val search = Var(initial = "")
val items = Vector("Foo", "Bar", "Baz", "Apple")

val content = div(
  input(value <-- search.signal, onInput.mapToValue --> search.writer),
  ol(
    items.map(txt =>
      li(display <-- search.signal.map(s => if txt.toLowerCase.contains(s.toLowerCase) then "block" else "none"), txt)
    )*
  ),
  router.currentPageSignal.map(_.getOrElse("")) --> search.writer,
  //
  // uncomment any one of the below to get a cycle:
  //
  // search.signal.map(Some(_).filterNot(_.isEmpty)) --> router.pushState,
  //
  // search.signal.changes.map(Some(_).filterNot(_.isEmpty)) --> router.pushState,
  //
  // search.signal.changes
  //    .debounce(1000)
  //    .map(s =>
  //      println(s"pushing $s") // no console error now, but there's still an infinite loop
  //      Some(s).filterNot(_.isEmpty)
  //    ) --> router.pushState,
  emptyNode
)
def main(): Unit =
  renderOnDomContentLoaded(org.scalajs.dom.document.querySelector("#appContainer"), content)

[ivan-klass]

Maybe just .pushState should be idempotent? I mean, do nothing if the second call has the same argument (i.e. state is already as provided) I don't think there's any practical reason to write the same entry into history again. It would also break that cycle, I believe.

[raquo]

Thanks, that helps me understand the use case.

In your code, you have an infinite loop that can be concisely expressed as router --> search + search --> router. Of course, this loop needs to be broken, so that it terminates. But, I'm not sure that breaking that loop behind the scenes (in router or pushState) is necessarily a good idea, as doing so would break the general expectations of how pushState works (native JS pushState does not have any filters in it), and how Airstream observables work (signals don't automatically do distinct since 15.0.0).

So, such loop breaking should probably be explicit, opt-in. Ideally we would use Airstream's built-in operators like distinct, but we can't, really, because what we need to distinct is the history system's internal state, not any signal's state.

Could we add a distinctCompose argument to Router's constructor, similar to the option in the split operator? Then you could say distinctCompose = _.distinct, and the Router would apply that transformation to currentPageSignal before exposing it. This would break the loop by means of currentPageSignal not emitting a duplicate event, but it would still execute pushState, and thus leave us with a stray entry in the history. So it seems that this is not the right place to put it.

Looking at your code, the following stands out to me as a potentially good place to fix the issue conceptually:

router.currentPageSignal.map(_.getOrElse("")) --> search.writer

It should be instead:

router.currentPageSignal.map(_.getOrElse("")) --> search.writerDistinct

But, writerDistinct (that would only trigger a Var update if the value is not equal) does not currently exist. We could implement it for Var-s, but other types of observers wouldn't have access to their current state, to distinct against. So, this would work for this example, but I'm not 100% sure that this would be a universal solution.

Hrm. Not super happy with any of that.

Generalizing a bit in the other direction, I guess we could add some kind of preventNavigation: (currentPage, nextPage) => Boolean configuration option to the Router constructor. Users would need to specify _ == _ manually if they want to block navigation calls to the "same" page – it has to be explicit. Maybe eventually this functionality could even grow to support use cases like https://github.com/raquo/Waypoint/issues/14, although that would need a bunch more work.

Thoughts?

---

Also – I do wonder if there is a valid use case for calling pushState with the current page. Could people be using it to "refresh" the current page? It doesn't seem like it would work well with typical waypoint usage patterns (SplitRender stuff), but not sure.

[ivan-klass]

The most confusing part for me was that .changes do not stop the cycle. It turned out that name gave me a misleading understanding that only values different from previous would be emitted, but this doesn't seem to be so.

I like the idea of preventNavigation, or maybe an isomorphic allowNavigation with default (_, _) => true. Say, we have a navigation graph like val navigationMap: Map[Page, Set[Page]] = ??? and have allowNavigation = navigationMap.getOrElse(_, Set.empty)(_). Or, say, we can ensure that user has read a new usage policy first.

I know there's Var.updater, what about an extra alternative like

extension [A](v: Var[A]) // just to test it compiles, can be added as native methods
  // the name inspired by `Map[A, B].updatedWith`
  def updateWith[B](f: (A, B) => Option[A]): Observer[B] = Observer[B](onNext = b => f(v.now(), b).foreach(v.set))
  def distinctWriter: Any = updateWith[A]((a, b) => Some(a).filterNot(b.==))

At the end, just curious, what could be any practical use of variable signals / writers to emit/process same values as previous?

[raquo]

Yes, the name .changes is back from the time when Signals had .distinct behaviour built-in, so the changes, would, in fact, be de-duplicated. I've been wanting to rename it to .updates or something that does not imply distinctiveness.

what could be any practical use of variable signals / writers to emit/process same values as previous?

Most use cases don't care one way or the other, but prior to 15.0.0 we've had problems with doing automatic .distinct within Signals, which were made worse by the inability to disable that behaviour once it's baked in. Both ergonomics and performance suffered in certain cases.

See https://laminar.dev/blog/2023/03/22/laminar-v15.0.0#no-more-automatic--checks-in-signals

I know there's Var.updater, what about an extra alternative like (...)

Yes, that's more or less how it should be implemented. Need to think about the name for updateWith that would fit in with the other Var methods, keeping in mind that we'll need updater and Try varieties as well.

---

More on the immediate subject matter, I just realized that preventNavigation would also need to apply when the navigation is triggered externally (e.g. using back button), and that is a more similar to #14 than I hoped, with the same complications as I outlined there. I guess a simpler version would be add an optional distinctFn argument to pushState, but I'm not loving that solution as it would be temporary until the abovementioned #14 functionality lands.

[ivan-klass]

II agree that it might have sense not to have .distinct by default in generic Signal - as it may represent a flow of events and triggers, not changes of values. But I'm doubtful about the Var specifically. It's used to store the state, and having a property that myVar.set(42); myVar.set(42); is equal to myVar.set(42) seems like something very intuitive, but this is misleading.

What if Var(initial = 42, ignoreSetSame = true) would exist? The performance impact should be extremely minimal I believe - it's just do nothing if the .set argument is already the current value, right? It will result in distinct var signal also.

[raquo]

What if Var(initial = 42, ignoreSetSame = true) would exist?

You know, that's a very good idea. I think perhaps we could have syntax like:

val distinctVar1 = Var(42).distinct
val distinctVar2 = Var(foo).distinctBy(_.id)

And so on, matching Airstream's distinct* operators. These methods on Var would create a LazyDerivedVar with a special zoomOut method that prevents writes to the parent Var when the new values are not distinct from the current value.

This syntax would provide a more flexible API to users, e.g. you could create an always-distinct Var like I did above, or have just one of the paths into the Var distinct, as below:

val myVar = Var(42)
onClick.mapTo(1) --> myVar.distinct // these updates are distinct
onClick.mapTo(foo) --> myVar.updater(_ + _.size) // but these are not

And in other cases we could still do --> myVar.distinct.updater(_ + _.size) if that made sense, so the update methods don't need to know about distinct.

Do you think this would sufficiently help out with your use case to not need your workaround, until we do a proper blockNavigation config in #14?