Open isaac-udy opened 5 years ago
Personally I think that if router {
already receives a Router.() -> Unit
, then the infix
is not needed.
@Zhuinden I agree
I think you raised a few very interesting points here!
Multiple paths to one goal:
1.
router.clear()
router.push(Route())
Exactly, this will work but won't bundle those instructions to a single which might break animations.
2.
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?
router { clear() push(Route()) } This one will ignore the result of the clear() call
:thinking:
That's totally not expected. I believe that block should be a "instruction chain builder".
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?
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) }
? 🤔
Or would you propose that the following syntax:
router {
clear()
push(route1)
push(route2)
}
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?
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.
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.
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?
That at that point there is no reason to have it passed as a lambda.
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?
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.
I will now build two branches:
infix
removedrouter {
clear()
push(route1)
push(route2)
}
Let's decide afterwards based on both working branches!
I'd like to start a bit of a discussion around the DSL syntax.
Multiple paths to one goal:
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:
Builder syntax:
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:
I have two questions:
infix
be removed, so a user can split the continuation over multple lines?