Kraigie / nostrum

Elixir Discord Library
https://kraigie.github.io/nostrum/
MIT License
622 stars 129 forks source link

Allow more than one api client per application/move config out of config.exs #134

Open lambdadog opened 5 years ago

lambdadog commented 5 years ago

Reading from config.exs is bad practice for a library in general, but this both prehibits nostrum from being used to create a hedwig adapter and for a large spectrum of use-cases, including, for example, discord bots as a service, where users can input their own token, etc, and set up a bot under a custom bot account for their server.

Kraigie commented 5 years ago

Thanks for opening this up! Sorry, but this will be a bit of a read. 🙃

I completely agree that reading from a config file is generally not a good idea for a library, there's even a section on it here in the official elixir docs.

That being said, because we're running nostrum as an OTP application I believe there's not much we can do in this department. If you've used something like phoenix before, you should be accustomed to including things like tokens and API keys in your config (although our config system is not nearly as robust).

As for the hedwig adapter, in the past someone has tried to hook up nostrum to hedwig with little success, maybe for the reasons you're mentioning here. I'm not keen on the benefit of using something like hedwig or how their adapters are implemented, does it just add abstractions for sending/receiving messages?

For your other use case, I'm not sure how TOS-friendly (depending on the implementation) the mentioned use case is. This is something I want to take into account when maintaining this lib. Although any changes we make (if any) as a result of this issue will just be coincidental in allowing this behavior I want to bring the issue to your attention if this was something you were trying to do.

Regarding the API client, if you're referring to just the API portion of nostrum i.e the REST-ful portion, it's already possible to do what you mention. If by API you mean the library as an entirety, which is the more likely case, then I'd like to ask what your use case here is.

Nostrum isn't like most other libraries in that it has the idea of a client. Instead we use the gen_stage pipeline, a more OTP-centric approach, with the user handling just the consumer portion of the pipeline. I think this lends itself to the application style of library that we have, as the user is just managing their consumer process, and nostrum handles all of itself. Personally I can't see a reason you'd want to run multiple clients under one application. If it's for sharding, then the ability to do that is already there. As for any other OTP shenanigans like distribution, we wouldn't handle that very well right now anyways. If I'm missing something here, feel free to let me know.

All that said, I'm pretty open to this change if there's a solid reason for it. We could move the config into ETS or something of the same ilk, or we could provide a more traditional approach where the user has to start nostrum as a whole under their own supervision tree.

Let me know your thoughts on the matter!

lambdadog commented 5 years ago

Yeah, I looked into the ToS after opening this issue and it seems you're not supposed (allowed?) to share API keys, so you're right on that front, I've decided to have the "bring your own bot user" bit stick to Twitch for now, as that's clearly not against their ToS looking at other similar services.

My use-case for integrating with Hedwig though is that I'm working on a multi-platform (/cross-platform, in the sense of different messaging platforms, not operating systems) "bot as a service" application, which means that I'm going to want to be able to reuse code, for example, between a twitch bot and a discord bot, and any more platforms that will come in the future. Hedwig's architecture lets me do this more easily, and also allows me to easily spawn as many bots as is dictated by the _aaS design.

In regards to the gen_stage pipeline, I noticed it poking through the library but honestly don't know much about it, so I'll have to do some more research in regards to that bit of the OTP.

That said, if I'm going to be running only one discord bot in the end, it almost sounds like it would be reasonable to create some kind of "virtual discord bot" adapter for Hedwig that would let me treat the discord bot on each server as an entirely different "virtual bot" and dodge this issue entirely, and this might even be necessary if I want people to be able to set up different capabilities and features for the discord bot for their own server, at least within the framework that Hedwig provides.

jvantuyl commented 4 years ago

I think there's probably a useful middle-ground here. I have a use-case or two that I think of as very mundane, but aren't really possible with Nostrum today.

Use Case 1

I want to provide a bot that has a web interface for configuration. Think of the 1,001 apps out there that you hit the UI for the first time, configure it, and then it becomes whatever it's supposed to be. This is a very sane and useful feature to have for a Discord bot.

Examples: DokuWiki, WordPress, openHAB, etc.

Use Case 2

I have an existing app, and I'd like to add Discord integration. Right now, it's pretty hard to do this without disruption for anything that doesn't have the bot be its primary use-case. If one want's to make something that has Discord functionality that's optional, it's very difficult given that Nostrum won't even let your app start without significant workarounds.

Example: Home Assistant, Beyond20, etc.

Proposal

What I propose is:

This gets you the following behavior:

I'd be glad to implement this if you'd accept it. Probably could turn it around quickly.

Embedding Example

For a real-world example of something that allows this kind of embedding, you can check out ranch (docs here). They also do a good job outlining why you might want to embed it, as well.

khionu commented 3 years ago

Just read this issue. This is superseded by #317's point of moving away from being an OTP Application, as well as #326 which cites the Elixir docs and the best practices of libraries importing configs.

blueforesticarus commented 2 years ago

I notice the alchemy library has this same issue with global architecture.

Is this a common thing with elixir libraries?

khionu commented 2 years ago

Whether a library should be an OTP Application is dependent on whether it makes more sense to be instance-oriented or reuse a single instance. For logger, it makes no sense to duplicate any background processes, all Applications should just log to the same instance of the program's observability sink. For any sort of API wrapper, there is the possibility for wanting to connect to the API multiple times, or to have better control over the lifecycle of the wrapper, so making the entire thing an Application doesn't make sense. That said, if there was some part of Nostrum that would make sense to be global, an option would be to have that shared portion in an Application (though it may make more sense to just put it in a named/global ETS table or something).

You can think of Applications as top-level services within a BEAM runtime, that other Applications may depend on. The catch is just, as this issue is about, that you can only have one copy of a given Application running, and that it can add comprehension overhead to people still learning Elixir. Personally, I don't think that should inform such an important architectural detail as whether to use an Application or not, but I've made my case already.