Zhuinden / simple-stack

[ACTIVE] Simple Stack, a backstack library / navigation framework for simpler navigation and state management (for fragments, views, or whatevers).
Apache License 2.0
1.36k stars 76 forks source link

Determine if `ScopeKey.Child` should have exposed a `List<ScopeKey>` instead of `List<String>` this whole time #209

Closed Zhuinden closed 4 years ago

Zhuinden commented 4 years ago

The original premise was that if I exposed a List<ScopeKey>, there would be no guarantee that it is actually Parcelable.

Therefore, it was safer to expose a List<String>.

However, now every key must be able to expose the parent scope's dependencies, possibly through a shared interface, which is alright as it works, but 9 out of 10 devs would hate it.

Also, the parent scopes also receive the first key that described the parent scope, but if it was a List<ScopeKey>, it would get the scope key instead. That way it could have implemented HasServices and wouldn't need an if to determine if it's your own services or your parents' services if you describe the services you bind inside your key.

Nonetheless, swapping out a List<String> for List<ScopeKey> and exposing that ScopeKey inside serviceBinder.getKey() would be heavily breaking for the current way of doing things, so this is more-so just thinking about nodes that don't entirely exist yet.

Zhuinden commented 4 years ago

The trick is that then you'd probably have to pass the "parent scope key" to the next screen anyway, basically a flat inheritance hierarchy where you initialize the parents through arguments passed to the children.

Reminds me of the "table per concrete class" approach.

I'm just rambling. Honestly, I'm probably not going to revise the way it is now.

matejdro commented 4 years ago

Maybe I am completely misusing this feature, but at the moment I am using services like that:

  1. Every Key defines services that it needs via Child.Key interface:
@Parcelize
class Key() : BaseKey(), ScopeKey.Child {
    override fun getParentScopes(): List<String> =
        listOf(
            Service1::class.java.name, 
            Service2::class.java.name
        )

        ...
}

(I never use non-child version of ScopeKey)

  1. Then every screen just retrieves the services it needs via Backstack.lookupService

Moving from String to any other class would make this much more complicated. Am I doing this completely backwards from the way I should be doing?

Zhuinden commented 4 years ago

I mean, if you bind Service1 in the scope called fqn.Service1, and you bind Service2 in the scope called fqn.Service2, and so on, then you can most definitely register 1 service per 1 scope and it'll work correctly (I know because I have a pretty good idea of the things I verify in tests)

The only thing that could be your enemy is if you need 2 instances of the same Service because you can have multiple of the same scope, for example in our case we had a UserProfileScope but you could open someone else's profile from it, so we had to make the scope tag parametric. We were using regular ScopeKey and created a scope tag like UserProfileScope[$userId], and bound services (UserProfileViewModel etc) in that scope.

We are using ScopeKey scope name as key class name as default, then change it if it needs to be parametric.

Your approach to explicit parent scopes makes it significantly easier to manage the service binding to an explicit parent scope tag. I've never considered using a service name as the name of the scope it resides in.

Now I'm definitely glad I made it a List<String> instead of List<ScopeKey>. Thank you for sharing your approach.

You might be "misusing" in the sense that it's not the way I originally imagined it, but it's actually way cooler than my construct of it. In my head (and in our code 🤔) , the List<String> could contain things like "registration" or "onboarding". Binding the RegistrationFlow as the name eliminates the need for the scope tag, the one thing that can potentially remain is binding aliases (my samples call it rebind as an extension function) to expose the class under various interfaces' class names.

Keep your way, I think it's great, and the explicit lookup direction will do exactly what you want ❤️

Zhuinden commented 4 years ago

@matejdro your idea is amazing so I put together a PoC where I made it work across multiple modules using Dagger: https://github.com/Zhuinden/simple-stack-multi-module-experiment/commit/94f5c2d959df5ca63704d03c3b862714fdabe22a