adrielcafe / voyager

🛸 A pragmatic navigation library for Jetpack Compose
https://voyager.adriel.cafe
MIT License
2.5k stars 126 forks source link

Re: Wanted to show you project I made with your library #46

Closed JohnBuhanan closed 2 years ago

JohnBuhanan commented 2 years ago

Hi @adrielcafe, I wanted to show you my sample app I made with Voyager: https://github.com/JohnBuhanan/MVI-Public

I studied a dozen github repos and felt like yours was closest to what I wanted.

Three things I thought you might find interesting:

  1. I wrapped your Navigator so that I can do routing from ViewModels.
  2. Each feature contains an api gradle module and an impl gradle module. The impls can be commented out from settings.gradle and app.gradle to unload them from Android Studio indexing and gradle builds. This allows scaling for massively multi-module apps.
  3. I used reflection to get access to the "internal factories" of ScreenRegistry.

Biggest thing I am thinking about now is best way to "navigate for result".

ynsok commented 2 years ago

Hi @JohnBuhanan

Do you know why it is not working without reflection of ScreenRegistry?

JohnBuhanan commented 2 years ago

@ynsok I think there is type-erasure happening with the way I do Routing :(

Maybe there is a better way to do routing from ViewModel.

ynsok commented 2 years ago

Generally, I think it should be enough, and solve our problem. What do you think? @adrielcafe @DevSrSouza @JohnBuhanan

     fun get(provider: ScreenProvider): Screen {
        val factory =
            factories[provider::class] ?: error("ScreenProvider not registered: ${provider::class.qualifiedName}")
        return factory(provider)
    }
JohnBuhanan commented 2 years ago

@ynsok You are saying we should add that function to ScreenRegistry?

I think you're right. That would make it so I don't have to do reflection.

I wonder how @adrielcafe feels about navigating from the ViewModel though?

I think it is highly desirable, but others might see it as an anti-pattern.

adrielcafe commented 2 years ago

I've implemented the @ynsok suggestion in 1.0.0-beta16, please reopen the issue if that was not enough.

About navigating from ViewModel/ScreenModel or any other class, I like the idea, some times that kind of logic is too complex and should not be in the UI layer. Feel free to open another issue about that, I'll start to think about an implementation.