CoordinatorKit icon indicating copy to clipboard operation
CoordinatorKit copied to clipboard

Memory leak when setting navigation controller delegate on view controller.

Open pavankataria opened this issue 6 years ago • 2 comments

Like with all the other examples and yours, setting the view controller as the delegate to the navigation controller delegate property breaks your closure link responsible for deallocating the child coordinator.

To reproduce, replace this code block into the ProfileCoordinator.swift file and toggle on and off the setting of the navigation controller delegate property in the ProfileCoordinator class to see it in action.

class ProfileViewController: UIViewController, UINavigationControllerDelegate {
    deinit {
        print("deinitialised")
    }
    override func viewDidLoad() {
        super.viewDidLoad()
        self.view.backgroundColor = UIColor.red
        
        //Toggle this line on and off as a comment to see that this view controller is never deinitialised.
        self.navigationController?.delegate = self
    }
}

class ProfileCoordinator: Coordinator<DeepLink> {
	
	let viewController = ProfileViewController()
	private let store: StoreType
	
	init(router: RouterType, store: StoreType) {
		self.store = store
		super.init(router: router)
	}
	
	// We must override toPresentable() so it doesn't
	// default to the router's navigationController
	override func toPresentable() -> UIViewController {
		return viewController
	}
}

pavankataria avatar Jun 04 '18 12:06 pavankataria

Hi, @pavankataria! Correct me if I'm wrong but shouldn't the Router be the delegate for the Navigation Controller and not the View Controller itself?

jcyu0208 avatar Jun 21 '18 06:06 jcyu0208

@jcyu0208 exactly! One of the fundamental reasons for coordinators is separating responsibilities. With this comes the idea that a child should not know or care about its parent, but merely send events back to its parent (via delegation). A view controller has no business making calls directly to its parent (navigation controller). @pavankataria maybe a broader view of what you're trying to accomplish would give better context for how you should be attempting to do it using coordinators?

imaccallum avatar Jul 13 '18 01:07 imaccallum