Improve type-safety for the total routes.

[arturaz]

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

[raquo]

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]

Restructured as requested.

[arturaz]

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]

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]

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]

Added compilation tests.

[arturaz]

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

[raquo]

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.

[raquo]

@arturaz Thank you for working on this, and for the patience! I've merged this now, and I'll include this in the next version.

I looked it over again after checking it out, and noticed that after all the revisions, getClass is not needed. I replaced it entirely with ClassTag, which we were using anyway.

I'm not 100% sure what type information Java's Class encodes, but I believe the change is 100% equivalent, because:

1. in case of the singleton (staticTotal), the Page type can not contain generic params, so it's a simple straightforward case
2. in all other cases, we used to get Class from ClassTag, so they must contain the same information.

If I'm missing something, and you intended to use Class for some of its capabilities that I'm not aware of, please let me know.

[arturaz]

Looks good to me!

---- On Sat, 09 Nov 2024 13:16:10 +0200 Nikita Gazarov @.***> wrote ---

https://github.com/arturaz Thank you for working on this, and for the patience! I've merged this now, and I'll include this in the next version.

I looked it over again after checking it out, and noticed that after tall the revisions, getClass is not needed. I https://github.com/raquo/Waypoint/commit/0218c7ea4be69807dc301cfb7193b6c63b8f0545#diff-72e107b3cc8eec584380b80e861f353e3c2f181372f6925272f0c5a8acfd339d it entirely with ClassTag, which we were using anyway.

I'm not 100% sure what type information Java's Class encodes, but I believe the change is 100% equivalent, because:

in case of the singleton (staticTotal), the Page type can not contain generic params, so it's a simple straightforward case

in all other cases, we used to get Class from ClassTag, so they must contain the same information.

If I'm missing something, and you intended to use Class for some of its capabilities that I'm not aware of, please let me know.

— Reply to this email directly, https://github.com/raquo/Waypoint/pull/19#issuecomment-2466176682, or https://github.com/notifications/unsubscribe-auth/AAADFA3E3HDS22DYOXJIPSTZ7XVHVAVCNFSM6AAAAABJTWBHMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRWGE3TMNRYGI. You are receiving this because you were mentioned.