DEV: Refactor presence manager to deal with multiple composer states. (PR #9594)

GitHub

Not sure how I can get around having to litter this every where to handle the case where the composer does not have a topic.id. Mainly when creating new topics.

I wonder if this would be better as an Ember service? That seems to be the most ‘ember-y’ way to create a singleton, and it’s how I handled a similar problem in discourse-whos-online. Then you can inject the service into components as needed. That would avoid the need for getOwner(this).lookup("presence-manager:main"))

totally support changing this, feel free to make the refactor, it is a bit fiddly from what I can tell and I really want to get these fixes in today so I will merge this anyway once I test it.

@davidtaylorhq what are the differences between factories and services? I was mainly following Dependency Injection - Application Concerns - Ember Guides to register an singleton with the application.

OK merged this in by hand, thanks @tgxworld really happy with this now … we can clean up the factory stuff next week I guess, but it is not urgent.

@tgxworld I think Services are the ‘easy-mode’ for singletons. Factories are ‘hard-mode’, but more flexible. From the link you shared

Generally, Services are Ember’s primary method for sharing state via dependency injection. In most cases, you shouldn’t need to learn about how to work with Ember’s DI system directly, or how to manually register and setup dependencies.

Nothing wrong with factories though, so this is totally fine to leave as-is for now.

Thank you for the explanation :+1: Do you happen to know how to initialize the service with attributes? I was looking at discourse-whos-online/online-service.js.es6 at master · discourse/discourse-whos-online · GitHub and it seems to be grabbing messageBus and currentUser from globals and it isn’t clear from the Ember docs whether that is possible

Hmm you’re right - the global lookup is not pretty! I guess it should be possible to use inject() there rather than a global lookup.

Maybe we should be adding currentUser and messageBus to services via inject-discourse-objects. :thinking:

O yea we should probably do that. TBH though, I do feel that services and factories are more or less the same as least in the context of how I’m using it :grin: Replacing getOwner(this).lookup("presence-manager:main")) with presenceManager: inject() and not having to register the factory manually wasn’t that painful.

Any thoughts about this @eviltrout? I feel like I might be missing something on why directly registering factories can be bad?

I do feel that services and factories are more or less the same as least in the context of how I’m using it :grin:

Agreed, nothing bad about it. In a perfect world I think siteSettings and appEvents would also be ember services. I even found a TODO for it :wink:

appEvents and currentUser should absolutely be services too and I would very much like to fix that eventually.

@tgxworld we should be minimizing getOwner and lookup - those APIs are private-ish and are discouraged in the community unless you absolutely have to use them. Over time I have been trying to remove as many as possible outside of initializers.

For now, we should go with @davidtaylorhq’s suggestion of injecting currentUser and appEvents into services in the initializer that does the other injecting automatically. That way it keeps the lookup apis in one place and not in various objects spread throughout our project.