Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
JohnBuhanan opened this issue Feb 10, 2022 · 5 comments
Closed

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

JohnBuhanan opened this issue Feb 10, 2022 · 5 comments

Comments

@JohnBuhanan
Copy link

JohnBuhanan commented Feb 10, 2022

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
Copy link

ynsok commented Feb 15, 2022

Hi @JohnBuhanan

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

@JohnBuhanan
Copy link
Author

@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
Copy link

ynsok commented Feb 16, 2022

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
Copy link
Author

JohnBuhanan commented Feb 16, 2022

@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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants