krzysztofzablocki / Inject

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

Using hosted instance of Inject.ViewControllerHost #24

Closed MarcoEidinger closed 2 years ago

MarcoEidinger commented 2 years ago

Hi

in README the following example is given

// WRONG
let viewController = YourViewController()
rootViewController.pushViewController(Inject.ViewControllerHost(viewController), animated: true)

// CORRECT
let viewController = Inject.ViewControllerHost(YourViewController())
rootViewController.pushViewController(viewController, animated: true)

Existing UIKit projects might want pass the viewController around, e.g. for configuration and then presentation.

let viewController = Inject.ViewControllerHost(YourViewController())
configureAndPresentReusableViewController(vc: viewController)

func configureAndPresentReusableViewController(vc: YourViewController) {
  vc.defaultText = "Configured"
  self.present(vc, animated: true)
}

The code above will not compile due to Cannot convert value of type '_InjectableViewControllerHost<YourViewController>' to expected argument type 'YourViewController'

If I use instance property of class _InjectableViewControllerHost<Hosted: InjectViewControllerType>: InjectViewControllerType then I can prevent the compilation error.

let viewController = Inject.ViewControllerHost(YourViewController())
configureAndPresentReusableViewController(vc: viewController.instance) // fine :)

func configureAndPresentReusableViewController(vc: YourViewController) {
  vc.defaultText = "Configured"
  self.present(vc, animated: true)
}

But I noticed that this either leads to incorrect visualization or to a runtime error down the road.

Any recommendation?

krzysztofzablocki commented 2 years ago

You'd want to create and configure in a single call and wrap that call with the host, then you present the host. That way when we inject you configure new instance again.

let vc = Inject.ViewControllerHost(createAndConfigureX())

present(vc)

So you'd add a func like that, we could also add a standard closure factory variant for hosts to the library as that could be easier to use in cases like this

MarcoEidinger commented 2 years ago

Got it, thanks. In one of the app we have this pattern quite a lot. Looks our app needs some refactoring to use Inject properly.

One remaining question: the incorrect way of not calling initializer inside Inject.ViewControllerHost(...) or Inject.ViewHost(...) will not properly hot-reload the presented user interface.

// WRONG
let viewController = YourViewController()
rootViewController.pushViewController(Inject.ViewControllerHost(viewController), animated: true)

But if I navigate away and back to the UI, essentially forcing the initialization process outside of Inject.ViewControllerHost(...) or Inject.ViewHost(...) then I will see the changes because InjectionIII.app previously did inject the changed code through the dynamic linker. Not instant hot-reloading but this might be a compromise if refactoring as above is not desired due to efforts.

Any concerns with this approach (e.g. memory leaks) besides loosing the instant hot reloading?

krzysztofzablocki commented 2 years ago

You should be fine but that workflow adds quite a lot of overhead due to the manual steps required.

I use configure and present separation in my flow coordinators and that worked well with host approach, but even if you don't want to refactor existing code but need to do a bunch of changes on a particular screen I'd just change that part in separate commit I'd revert as it would pay for itself in few iterations :)

MarcoEidinger commented 2 years ago

Thanks for all your input! That was super helpful :)