Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The singleton pattern is encouraged, but doesn't support multiple callbacks #9

Closed
colinta opened this issue Oct 31, 2016 · 4 comments
Closed

Comments

@colinta
Copy link

colinta commented Oct 31, 2016

Since callbacks only stores one closure of each event type, multiple controllers that use Typist.shared will "fight" over who receives notifications.

class ControllerOne: UIViewController {
  override func viewDidLoad() {
    Typist.shared.on(event: .willShow) { _ in print("will show from one") }
  }
}

class ControllerTwo: UIViewController {
  override func viewDidLoad() {
    Typist.shared.on(event: .willShow) { _ in print("will show from two") }
  }
}

If both of these controllers are loaded, only the second controller will get the notifications.

Easy fix: remove the singleton, and have each controller retain its own Typist instance.

@MichalAlgor
Copy link

The current solution let you do that. You don't have to use shared singleton, just do as you proposed. Use different instances of Typist in your controllers.

@colinta
Copy link
Author

colinta commented Oct 31, 2016

Right, but encouraging the use of the singleton will lead to bugs. The "correct use" should be the one that is documented in the README. As it is now, the README doesn't mention this bug/limitation. Your package was included in "little bites of cocoa" and the author there also didn't point out that using the singleton is a bad idea, probably because it's not mentioned in the README.

@totocaster
Copy link
Owner

totocaster commented Nov 1, 2016

Hey Colin, what you described is true and I though that's was obvious, thus the comment in README about singleton vs new instance. Reason there is an option to use singleton at all is to give quick access to instance without having a dedicated class property (Typist().on(... would deallocate immediately.) Having said that, your argument is totally valid. I'll reword docs and mention this limitation when working with singleton. —Thank you!

@totocaster
Copy link
Owner

Updated README. This is not final decision though, I'll think more about singleton usage and it's benefits vs confusion factor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants