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

Allow MainActor Dependency Registration #225

Open
wants to merge 3 commits into
base: swift6
Choose a base branch
from

Conversation

tareksabry1337
Copy link

@tareksabry1337 tareksabry1337 commented Sep 4, 2024

What this does?

I have been reading through all of the issues related to MainActor isolation / MainActor resolution on Factory and there were some clear concerns that this is not the concern of Factory, while I kind of agree but I think Factory should be able to adapt to different approaches, whether you choose to either conform your entire ViewModel to @.MainActor or mark only certain functions, as long as you provide the correct definitions of your dependency registration / resolution, things should just work

How was this achieved?

  • Sendable requirement was relaxed in Factory / ParameterFactory / FactoryRegistration
  • But the registration on Container still enforces this Sendability
  • Introduce two new functions that are MainActor specific for registering dependencies that are MainActor isolated

Using these two functions we are allowed to do such declaration which isolates our dependency registration to the scope that they are defined on, in our case MainActor

Screenshot 2024-09-04 at 22 02 56

And now it's on the compiler to ensure that you are actually resolving MainActor dependencies only from MainActor bound classes, but you also may use non-MainActor bound classes in MainActor classes.

Example:

@MainActor
final class NotificationsViewModel: ViewModel {
    // MARK: DI
    @Injected(\.deepLinkHandlerViewModel) private var deepLinkHandlerViewModel // This is MainActor isolated and compiles
    @Injected(\.deepLinkGenerator) private var deepLinkGenerator // This is not MainActor and also compiles and the resolution works as expected

But if we were to remove @.MainActor annotation from this ViewModel, this would result into the following warning (soon to be error)

Screenshot 2024-09-04 at 22 10 38

Which means that we have achieved even more compile-safety when defining / resolving dependencies.

Let me know what do you think

@tareksabry1337
Copy link
Author

@hmlongco Appreciate your thoughts

@hmlongco
Copy link
Owner

hmlongco commented Sep 5, 2024

This appears to be fighting with Sendable conformance. It may solve MainActor issues, but at the expense of Sendable.

@tareksabry1337
Copy link
Author

Open to suggestions, if there's some way to maintain both, but this PoC kind of proves that we can achieve that, somehow.

@tareksabry1337
Copy link
Author

tareksabry1337 commented Sep 7, 2024

Also, I don't think I'm really fighting Sendable conformance as we currently have four entry points for the convenience initializer, you either choose @.Sednable and initialize non-MainActor bound objects, or choose @.MainActor (which is implicitly Sendable) and initialize your MainActor bound classes. The only caveat that self instantiating Factory / ParameterFactory will not have this Sendability check in its initializer, which I don't know how we would resolve except duplicating Factory / ParameterFactory to have "Sendable" versions, and restrict the main ones to be MainActor bound, which is not feasible obviously but that's top of my mind, let me know what do you think

Also to be clear I tried the following approach

    @MainActor
    var communityViewModel: Factory<CommunityViewModel> {
        let analyticsRepository = repositoryContainer.analyticsRepository()
        let courseRepository = repositoryContainer.courseRepository()
        let coordinator = coordinatorContainer.communityCoordinator()
        
        let viewModel = CommunityViewModel(
            analyticsRepository: analyticsRepository,
            courseRepository: courseRepository,
            bannerManager: .init(),
            coordinator: coordinator
        )
        
        return self { viewModel }
    }

Which seems fine since I'm initializing my MainActor bound objects in MainActor isolated context the variable, but that will never work for the ParameterFactory since the parameters are inside its closure, if there's a way to do this to ParameterFactory similar to what I'm doing here let me know.

@NachoSoto
Copy link

This breaks what was added in #218.

A better approach is perhaps #219.

@NachoSoto
Copy link

Also removing @Sendable makes it susceptible to https://t.co/wmd1UPnROE

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

Successfully merging this pull request may close these issues.

3 participants