krzysztofzablocki / Inject

Hot Reloading for Swift applications!
MIT License
2.14k stars 114 forks source link

Suggested simplified SwiftUI integration #20

Closed apptekstudios closed 2 years ago

apptekstudios commented 2 years ago

I've enjoyed the fast iteration afforded by this library, thanks for putting it together!

In the attached pull request is my suggestion for a slightly simpler SwiftUI integration. 1) Replaces adding @ObservedObject private var iO = Inject.observer with @Injection var inject. 2) Move callback config into enableInjection function

Not sure if everyone would agree on this approach, but feels a little cleaner and also allows stripping the 'ObservedObject' component from release builds.

johnno1962 commented 2 years ago

Thanks @apptekstudios, very very interesting. What do you think @krzysztofzablocki, @zenangst, @wojciech-kulik? Gets my vote!

zenangst commented 2 years ago

This looks super neato!

johnno1962 commented 2 years ago

Before you merge this, I have a PR outstanding upstream https://github.com/apptekstudios/Inject/pull/1

wojciech-kulik commented 2 years ago

I'm also thinking about one idea to even further simplify the approach for SwiftUI. I will check today if it is possible. If you can please wait with merging this :)

wojciech-kulik commented 2 years ago

I was trying to figure out how to limit the integration to just .enableInjection(). However, it doesn't work.

struct InjectWrapperView<Content: View>: View {
    @ObservedObject private var injection = Inject.observer

    var body: some View {
        content()
    }

    private let content: () -> Content

    init(@ViewBuilder _ content: @escaping () -> Content) {
        self.content = content
    }
}

public extension SwiftUI.View {
    func enableInjection() -> some SwiftUI.View {
        _ = Inject.load

        return InjectWrapperView { AnyView(self) }
    }
}

Unfortunately, enableInjection captures self and that prevents updates. When the observer publishes update, InjectWrapperView is refreshed but it uses the old version of hot-reloaded view.

Do you guys think it is possible to limit integration to a single line?

johnno1962 commented 2 years ago

It's interesting to muse about this. I spent quite a while getting down to two lines but it feels like something can be done better. Using a property wrapper is certainly a step in that direction. In practice it seems SwiftUI captures the result of the body property and won't call it again until one of the View struct's inputs has changed. I don't know how this is plumbed but it feels like something to do with onChange could be done perhaps from a protocol extension? I don't know.

johnno1962 commented 2 years ago

I'm looking at this article... https://www.hackingwithswift.com/quick-start/swiftui/how-to-run-some-code-when-state-changes-using-onchange but I'm not sure it leads us anywhere.

apptekstudios commented 2 years ago

I did actually get a very similar wrapper working initially that avoided any property wrapper at all, but with one catch...

SwiftUI will not recompute body unless one of the inputs changed, but you can force refresh by using the id modifier with the injection count

This works reliably however any state within the view will be reset. It's worth noting that this will happen anyway with the current implementation of the type of your view changes (as it's wrapped in AnyView)

wojciech-kulik commented 2 years ago

@apptekstudios could you show an example?

krzysztofzablocki commented 2 years ago

curious to see example too @apptekstudios

apptekstudios commented 2 years ago

I've pushed an updated version to try and get the best of both worlds Edit: didn't work correctly

johnno1962 commented 2 years ago

Thanks @apptekstudios, this is awesome! If you ask me option 3 should be the default.

apptekstudios commented 2 years ago

Hmm my test case was flawed, I think we still need the property wrapper to make this work :(

krzysztofzablocki commented 2 years ago

pretty sure we do, I actually tried id(injectonNumber) over a year ago when I was updating my wrappers but Swift wouldn't reload the content view

apptekstudios commented 2 years ago

PR now reflects the following 2 steps: 1) Add @ObserveInjection var inject 2)Call .enableAnimation at end of view body

For reference the issue with the id approach was that it works to reload nested views but not the actual content defined in the view body itself

wojciech-kulik commented 2 years ago

This is the same problem I posted above. I think there is no way to force parents to rebuild. SwiftUI updates only affected part of the UI tree. We would need to pass a closure to build a parent view and replace it, but it would increase the complexity of integration :(

krzysztofzablocki commented 2 years ago

I really don't think the current approach is complicated, especially since it's no-op in prod and you usually just leave this code be, and you can use custom Xcode template to just always start with it 😂

@apptekstudios can we remove the animation modifier for the method though? so keep just property wrapper changes The function approach isn't going to work well especially since we usually leave code in prod for all views and @zenangst implementation is just global state, you'd constantly overwrite in each view

johnno1962 commented 2 years ago

So sorry to see the possible elimination of the observer variable backed out; It so nearly worked. I got to the point where if you have an .enableInjection() at the top level and where you are injecting it worked but the resetting of View state and resulting pop of the navigation controller was a definitely a problem. Perhaps we'll get there one day.

Happy to see the property wrapper renamed to @ObserveInjection as @Injection seemed a bit generic. If this PR could be updated to just add this property wrapper for people to move to and leave the rest of the already released surface of the api unchanged it would be a great step forward 💯.

johnno1962 commented 2 years ago

As a final thought, why not just public let wrappedValue = 0 in both Debug and Release instead of the over elaborate public private(set) var wrappedValue: Inject.Type = Inject.self?

johnno1962 commented 2 years ago

👍