QuickBirdEng / XCoordinator

🎌 Powerful navigation library for iOS based on the coordinator pattern
MIT License
2.27k stars 179 forks source link

Fire transitions in Main Thread #113

Closed andrefmsilva closed 5 years ago

andrefmsilva commented 5 years ago

I think the current TransitionPerformer should check and switch to main thread if needed... It handles UI transitions and it should always be called on main thread.

When we use the route in some ViewModel is easy to forget that we need to trigger it on main thread, so it should be handled inside this framework.

pauljohanneskraft commented 5 years ago

I understand the idea and possible benefits regarding these changes.

However I would like to argue against this change due to the following reasons. This only reflects my personal opinion, so feel free to provide counter-arguments that may change my mind.

extension Router {
    func threadSafeTrigger(_ route: RouteType, with options: TransitionOptions = .init(animated: true), completion: (() -> Void)?) {
        if Thread.isMainThread {
            trigger(route, with: options, completion: completion)
        } else {
            DispatchQueue.main.sync {
                trigger(route, with: options, completion: completion)
            }
        }
    }
}

or even more reusable:

public func onMainThread<T>(_ block: () throws -> T) rethrows -> T {
    return try Thread.isMainThread
        ? block()
        : DispatchQueue.main.sync(execute: block)
}
andrefmsilva commented 5 years ago

I agree. I was already using an extension to trigger on main thread