krzysztofzablocki / Inject

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

Implemented auto loader for Inject #32

Closed wojciech-kulik closed 2 years ago

wojciech-kulik commented 2 years ago

What do you guys think about this solution? It removes the need of using Inject.load or .enableInjection() (unless you want live changes) :)

johnno1962 commented 2 years ago

Hi, This seems like a good idea and is effectively what the iOSInjectioBundle does on loading itself. I don't think you can encourage people to do without .enbleInjection() though as it also erases the type of content Views which is critical for reliable SwiftUI updates. It may appear to work most of the time but add a UI element that alters the concrete type underlying the Opaque some View and you'll get a crash.

wojciech-kulik commented 2 years ago

Yes, enableInjection will be required for SwiftUI if you want live changes. However, for example in UIKit project I don't always need live changes, what's more, I don't always use Inject for UI hot reload, sometimes just for tweaking code and thanks to these changes I won't have to change anything in my code to enable Inject. Before I had to add in AppDelegate _ = Inject.load.

The same applies to SwiftUI, now if you want to have a hot reload, you need to navigate to a screen that uses .enableInjection or enable it explicitly by _ = Inject.load. This way you have hot reload enabled as soon as app starts :)

johnno1962 commented 2 years ago

Yeah, I agree this is a good idea. I'm very sensitive to downplaying the role of .enableInjection() (which was called .eraseToAnyView() in a former life) as not using it may make the tech seem unreliable. So, it's a thumbs up from me for the PR this aside 👍 . BTW, It's no longer documented but you can use the environment variable DYLD_INSERT_LIBRARIES in your scheme to load the injection bundle but to have a package you add load it automatically using Objective-C makes as much sense these days.

wojciech-kulik commented 2 years ago

I agree, that's why we should keep the information in README to always use .enableInjection() for SwiftUI views. However, this will be a nice addition having a hot reload when the app starts.

wojciech-kulik commented 2 years ago

Could you also run some tests on your side before merging? To make sure it doesn't break anything.

wojciech-kulik commented 2 years ago

@johnno1962 @krzysztofzablocki anything else to change or can we merge it? :)

johnno1962 commented 2 years ago

I have mixed feelings about this change. Sometimes it is useful to be able to opt in and out of loading the injection bundle from the source code if you suspect you may be experiencing some strange artefact and would like to eliminate it as a possible cause. This is @krzysztofzablocki's project however and he'll be the one making the call.

wojciech-kulik commented 2 years ago

I have mixed feelings about this change. Sometimes it is useful to be able to opt in and out of loading the injection bundle from the source code if you suspect you may be experiencing some strange artefact and would like to eliminate it as a possible cause. This is @krzysztofzablocki's project however and he'll be the one making the call.

Hmm, sounds reasonable. Maybe we could add some option to opt-out, but I'm not sure if it is possible with this approach. Or maybe after all it's better to just call Inject.load when you want it.

krzysztofzablocki commented 2 years ago

yeah I'm not convinced this is needed tbh, this only saves you 1 line now right? and it takes away ability to opt in/out easily

krzysztofzablocki commented 2 years ago

yeah I'm not convinced this is needed tbh, this only saves you 1 line now right? and it takes away ability to opt in/out easily

wojciech-kulik commented 2 years ago

Ok, so I think we can drop it then :).

In my case, I need it because I don't want to commit this line and I'm seeing changes in this file in git all the time. However, I've got this source code included locally anyway, so I can keep auto load in my case.

krzysztofzablocki commented 2 years ago

@wojciech-kulik curious why not have it commited if it's no op in prod?

wojciech-kulik commented 2 years ago

Actually, I think I could commit the code now :). No reason not to do it, I guess. Previously I was treating it more like an experiment, but now when we see it works well, we could commit it.