Discussion: DSL syntax

[isaac-udy]

I'd like to start a bit of a discussion around the DSL syntax.

Multiple paths to one goal:

router.clear()
router.push(Route())

router { 
    clear() push Route()
}

router {
    clear()
    push(Route())
}

There are several ways to perform similar actions. The first way isn't correct, as it won't bundle up the actions as a single item, but my understanding is the last two ways are essentially the same. ̛I think it would be a better idea to have a single syntax, even if it means that a single "push" needs to be performed as:

router { push() }

Builder syntax:

router {
    clear() push RouteA() push RouteB() push RouteC()
}

The builder continuation syntax that allows a user to push a series of routes like this is inconsistent with the use of infix functions, as a user is unable to split these onto different lines. The following code will not compile:

router {
    clear() push RouteA()
        push RouteB()
        push RouteC()
}

I have two questions:

1. Is the builder/continuation syntax actually useful? Could commands just be split onto different lines?
2. If the builder/continuation syntax is useful, should the use of infix be removed, so a user can split the continuation over multple lines?

[Zhuinden]

Personally I think that if router { already receives a Router.() -> Unit, then the infix is not needed.

[isaac-udy]

@Zhuinden I agree

[sellmair]

I think you raised a few very interesting points here!

Multiple paths to one goal:

router.clear()
router.push(Route())

Exactly, this will work but won't bundle those instructions to a single which might break animations.

---

router { 
    clear() push Route()
}

This will lead to the wanted behavior of bundling those two instructions to a single ✅

---

router {
    clear()
    push(Route())
}

This one will ignore the result of the clear() call. The Router.invoke function expects a function of type RoutingStack<T>.() -> RoutingStack<T>. The clear function returns a new RoutingStack. This new RoutingStack will be ignored by the push because it is received by this and not the result of the clear. So we can see that the syntax leads to confusion here and is not optimal!

---

Let me address your questions by clarifying the underlying architecture of the router a little more:

A Router contains a RoutingStack. This RoutingStack is nothing more than a list of routes (bundled with a unique key). Any instruction is nothing more than a function from a RoutingStack to a new RoutingStack : RoutingStack<T>.() -> RoutingStack<T>. So you have all the freedom in routing possible. You can do anything here. All the operators like pop, push, clear, ... are just defined on an interface called PlainStackInstructionSyntax which the RoutingStack implements.

The Router itself also implements the PlainStackInstructionSyntax, but in a way that you cannot chain instructions.

So that being said: When opening the router { } context, then chaining the commands is necessary!

Regarding the infix function: Theoretically one could use them just as normal functions, but I guess people tend not doing it then? But as from now, I think they are one of the major sources of confusion and we should rather remove them? Also we might want to make sure, that results are check inside a router instruction. Mabye @CheckResult could help us here?

[Zhuinden]

router {
clear()
push(Route())
}
This one will ignore the result of the clear() call

🤔

That's totally not expected. I believe that block should be a "instruction chain builder".

[isaac-udy]

Ah yes, number 3 tricked me too!

The clarification on the architecture was useful, I was obviously misunderstanding things a little bit.

What would you think if we made route { } execute as a single stack instruction, and use it as an "instruction chain builder" as Zhuinden suggests?

[sellmair]

What would you think if we made route { } execute as a single stack instruction, and use it as an "instruction chain builder" as Zhuinden suggests?

You mean like

router { clear().push(route1).push(route2) } 

? 🤔

[sellmair]

Or would you propose that the following syntax:

router {
    clear()
    push(route1)
    push(route2)
}

[sellmair]

Haha I get it 👍
I even had a draft supporting this kind of syntax at the beginning. The problem with it is, that it kind of doesn't fit into the (List<Route>) -> List<Route> signature which I really think is key for having an extremely solid router foundation. This signature is flexible in a way that it can cover 100% of possible routing scenarios and it is extensible in a way that it is easy to write custom operators on it 🤔

@Zhuinden @isaac-udy What are your thoughts on this one? At least, I am sure there would be no way of building this syntax while keeping the whole thing immutable.

Maybe the underlying concept would then more look like a
MutableList<Route> -> Unit inside this router { } lambda. Which might be fine. But personally, I prefer the immutable version List<Route> -> List<Route more. Maybe there is a way of building the syntax on top of it?

[isaac-udy]

You make a good point with the (List<Route>) -> List<Route> idea - this is very flexible, and very functional. I do like this as a concept. Currently the syntax used is more like List<Route>.() -> List<Route>, and I think the implicit receiver is what has caused Zhuinden and I some confusion.

I would suggest the following:

class Router { 
    ...
    @CheckResult(...)
    fun pushTransaction(val transaction: (List<Route>) -> List<Route) { ... }

    operator fun invoke(val transaction: MutableList<Route>.() -> Unit) { 
        pushTransaction { it ->
            return@pushTransaction mutableListOf(it).transaction().toList()
        }
    }
}

I think this is the best of both worlds - it gives the "easier" interface as the default router { ... }, but is performed through the functional interface, which is exposed if it is required. The toList call ensures that we use a MutableList in the router.invoke but doesn't expose it to modification after the call returns.

[sellmair]

Exactly! That was also what I had in mind, but I am not quite sure about it. Maybe one of my colleagues can contribute its thoughts on this tomorrow.

I honestly think the List<Route>.() -> List<Route>/functional style has more advantages and the most confusing thing might just be the infix function. Maybe removing the infix is enough?

The problem with the CheckResult:
The Router is part of the common Kotlin API, while CheckResult is just an Android annotation :/
Right now, I do not know something similar for Kotlin only.

[isaac-udy]

Maybe removing the infix is enough?

I think this is a good idea, but I think that we should also consider changing the signature of the invoke function - currently it's RoutingStack<T>.() -> RoutingStack<T>, but I think (RoutingStack<T>) -> RoutingStack<T> would be better, as it forces the user to write it.clear().push() ... rather than using the implicit receiver. What do you think?

[Zhuinden]

That at that point there is no reason to have it passed as a lambda.

[sellmair]

Another idea:
What about having something like:

router.clear().push(route1).commit()

Whoever wants to do custom routing with the List<Route>.() -> List<Route> can still open the lambda like router { clear().push(route1) }?

What are your thoughts on that?

[KlausNie]

I think the simple function chaining is the easiest to understand and fulfills the basic needs when using this lib. I also see how the lambda gives you more flexibility. So why not keep both?

Having 2 ways to do the same thing can be a cause of confusion and this should be clearly stated in the docs.

[sellmair]

I will now build two branches:

• One with the infix removed
• One with the syntax as

router {
    clear()
    push(route1)
    push(route2)
}

Let's decide afterwards based on both working branches!