(#717) Fix issue with deallocating view controller when replacing root view controller of UIWindow

[rafalwojcik]

Fix issue from ticket: #717

[JoeMatt]

Code loos good, though reviewing this I wonder if this should call the completion with an error or throw on error at least.

This method currently just consumes errors quietly. Suprised no one has complained about this.

[rafalwojcik]

@JoeMatt agree there should be some error handling.

Do you think about: completion: ((success: Bool) -> Void)? = nil or completion: ((Error?) -> Void)? = nil?

[dimatarelkin]

completion: ((success: Bool) -> Void)? = nil

Would you mind if I suggest one more variant?

public enum ReplaceRootViewError: Error { // smth like that not sure about naming
   ... // describe all your failure cases
}

public enum Status {
    case success
    case failure(ReplaceRootViewError) // or its can be just Error
}

func replaceViewController(with next: UIViewController, completion: ((status: Status) -> Void)? = nil) {
  ...
  // failure
  completion?(.failure(ReplaceRootViewError.someError))
  ...
  // success
  completion?(.success)
  ...
}

It would be Ideal to throw error as @JoeMatt said, to identify what was wrong.
You need only give descriptive names to these errors (the hardest part 🥲)

Also the last case with window should also call completion when transition finished (now its not).
And fix cases that just make return and telling nothing to function caller about completion (so caller that waits for completion never find out that function completed).

Hope, it was helpful :)

[JoeMatt]

Hope, it was helpful :)

This looks really helpful indeed, I'll take a look at incorporating as soon as I have the bandwidth or if someone else opens/updates a PR for review.

Thanks @dimatarelkin

[oconnelltoby]

@dimatarelkin, if you're thinking of using an enum to model the completion status, why not use Swift's Result?

public enum ReplaceRootViewError: Error {
   ...
}

typealias CompletionResult = Result<Void, ReplaceRootViewError>  // Could also use a plain Error here

func replaceViewController(with next: UIViewController, completion: ((result: CompletionResult) -> Void)? = nil) {
  ...
  // failure
  completion?(.failure(.someError))
  ...
  // success
  completion?(.success(()))
  ...
}

[dimatarelkin]

@dimatarelkin, if you're thinking of using an enum to model the completion status, why not use Swift's Result?

public enum ReplaceRootViewError: Error {
   ...
}

typealias CompletionResult = Result<Void, ReplaceRootViewError>  // Could also use a plain Error here

func replaceViewController(with next: UIViewController, completion: ((result: CompletionResult) -> Void)? = nil) {
  ...
  // failure
  completion?(.failure(.someError))
  ...
  // success
  completion?(.success(()))
  ...
}

Yeah, that would be nice, thanks @oconnelltoby