raquo / Waypoint

Efficient router for Laminar UI Library
MIT License
92 stars 11 forks source link

Improve type-safety for the total routes. #19

Open arturaz opened 3 months ago

arturaz commented 3 months ago

This should be source compatible but not binary compatible due to Route -> RouteImpl change.

raquo commented 3 months ago

Thanks! TBH initially I didn't realize that we would need to distinguish between partial and total routes to support the desired functionality, but it's obvious now.

I think your goal can be achieved in a simpler way: make Route into an abstract class, and have Route.Partial and Route.Total classes extend from it. The Route abstract class should have pretty much the entire implementation in it, with the Total and Partial subclasses containing only the methods that are specific to those use cases (probably none for Partial, and that one new method you want for Total).

I understand that this would be less flexible than using thin traits and thick impls, but I don't want to optimize for flexibility, I want to optimize for simplicity, and part of that is representing things the way we think about the domain problem. I don't think of routes as containing or deriving other routes (def backing), and I don't think of routes as something abstract that can have multiple implementations. All routes share the same implementation, total routes just have some extra methods. In this PR, it's hard to see that Route has shared implementation. It just... happens to be shared because one impl is calling into another, but the sharing is not a natural part of its structure the way a shared superclass is.

arturaz commented 3 months ago

Restructured as requested.

arturaz commented 1 month ago

I've made the requested changes and introduced Route.staticTotal. While it is possible to misuse it, you have to explicitly opt-in into it and I find it useful.

arturaz commented 1 month ago

Turns out there is a way to require a type to be singleton!

https://www.scala-lang.org/api/2.13.6/scala/ValueOf.html

Thus no extra tests are needed, right?

raquo commented 1 month ago

TIL, that's great, we should use that for the new method. Does it mean that the user won't be able to manually provide the incorrect AppPage type ascription anymore, eliminating the safety issue? If so, I guess we can even drop unsafe from the name. Still need some very basic test, including a doesNotCompile check on the incorrect usage pattern that you mentioned in the scaladoc, to prevent regressions.

arturaz commented 1 month ago

Added compilation tests.

arturaz commented 1 month ago

Just to clarify - is there anything left to do before merging this?

raquo commented 1 month ago

No, everything looks good, thanks! I'll merge this and will do some other maintenance when I have time to work on OSS, probably next month.