hmlongco / Factory

A new approach to Container-Based Dependency Injection for Swift and SwiftUI.
MIT License
1.7k stars 107 forks source link

onPreview modifier breaks shared scope #146

Closed rmickeyd closed 10 months ago

rmickeyd commented 10 months ago

The onPreview modifier seems to break the shared scope. Looking at the example below, when the onPreview modifier is commented out the DefaultManager will init exactly once and the same instance will be used in both the TestViewModel and DetailViewModel. When it is uncommented, the DefaultManager will be initialized in the TestViewModel and then a second instance will init/deinit with every push/pop of the DetailView.

I also tested this with other modifiers and the results seem to be consistent.

extension Container {
    var manager: Factory<Manager> {
        self { DefaultManager() }
            .scope(.shared)
//            .onPreview(factory: { PreviewManager() }) <--- uncomment to break shared scope
    }
}

protocol Manager {}

final class DefaultManager: Manager, ObservableObject {
    init() {
        print("init default object")
    }

    deinit {
        print("deinit default object")
    }
}

final class PreviewManager: Manager, ObservableObject {
    init() {
        print("init preview manager")
    }

    deinit {
        print("deinit preview manager")
    }
}

final class TestViewModel: ObservableObject {
    @Injected(\.manager) private var manager
}

struct TestView: View {
    @StateObject var viewModel = TestViewModel()

    var body: some View {
        NavigationStack {
            NavigationLink {
                DetailView()
            } label: {
                Text("Hello, World!")
            }
        }
    }
}

struct DetailView: View {

    @StateObject var viewModel = DetailViewModel()

    var body: some View {
        Text("Detail")
    }
}

final class DetailViewModel: ObservableObject {
    @Injected(\.manager) private var manager
}

There does seem to be a couple of options to get around this but I still think the above should work.

Option 1 - Autoregister:

extension Container: AutoRegistering {
    public func autoRegister() {
        #if DEBUG
        Container.shared.manager.onPreview(factory: { PreviewManager() })
        #endif
    }
}

Option 2 - once() modifier:

extension Container {
    var manager: Factory<Manager> {
        self { DefaultManager() }
            .scope(.shared)
            .onPreview(factory: { PreviewManager() })
            .once() // <--- added this
    }
}
hmlongco commented 10 months ago

Sigh. There are a lot of sides to this issue, including a previous one (#114), but you're right. Trashing the scope every time the Factory is resolved is not a good thing. :(

I removed the code that automatically clears the cache when setting a context and it's up in develop if you want to take a look.

A side effect of this is that if we ever want to change a Factory's context--but that Factory defines a scope--then we're also going to need to manually clear the scope cache for that object.

Container.shared.myService
    .onTest { NullAnalyticsEngine() }
    .reset(.scope)

I'm probably going to recommend that any context modifiers one wants to define should probably be in the autoRegister function as there's fewer side effects when you do so.

rmickeyd commented 10 months ago

Makes sense to me. I agree about autoregister, that is the option I decided to move forward with.