1

The review of SSWG-0023: DiscordBM begins now and runs for two weeks, until September 19th, 2023.

Reviews are an important part of the Swift Server Work Group incubation process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via email) or direct message in the Swift forums.

The package is being proposed in either the Sandbox or Incubating maturity level. Please note the Maturity Justification inside the proposal for details.

The previous pitch thread is here: DiscordBM

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, become listed on the Swift Server Ecosystem index page.

When reviewing a proposal, here are some questions to consider:

  • What is your evaluation of the proposal and its APIs/implementation?
  • Is the problem being addressed relevant for the wider Swift on Server ecosystem?
  • Does this proposal fit well with the feel and direction of the Swift Server ecosystem?
  • If you have used other libraries in Swift or other languages, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Joannis Orlandos
The Review Manager

4 Likes
2

Overall, I am +1 on incubating this package and I think it is worth filing this gap in the ecosystem.

Some comments on the code in the proposal:

The 2 most important concepts in DiscordBM are the GatewayManager and the DiscordClient protocols. Everything else in the library revolves around these two. Both of the protocols are just abstractions to make DiscordBM easier to test for users. The main types that conform to those protocols are BotGatewayManager and DefaultDiscordClient .

I think this is something that hasn't been done in the Swift server ecosystem yet and it is mostly expected that packages that use something are creating protocols to abstract a dependency for better testing. I would like to understand if there is another motivation for those protocols; otherwise, I think just providing a DiscordBotGateway and a DiscordClient would be better.

await BotGatewayManager.init(...)

The init of this type is async. Why is that the case and could we make it synchronous instead?

await bot.makeEventsStream()

Can we spell this as bot.events instead? Why do we need a factory method?

3

I have also noticed most Server Side packages don't come with even a few abstractions specifically for testing for users. The reason i came up with those protocols is that I wanted to write some tests for Penny at some point, and i thought why not have built-in testing helpers?
I do understand such protocols can be limiting if i want/need to expand the public API related to those protocols, but that has not been a problem.
There is no other motivation for the 2 protocols to be clear.

About DiscordClient, the plan is to replace it with a concrete type which instead just takes a DiscordClientTransport, in a new major version. I've noticed that pattern in some packages such as the openapi-runtime, and i think it would be the better way to do this.

otherwise, I think just providing a DiscordBotGateway and a DiscordClient would be better.

Are you (still) thinking that i should leave those testing parts to users themselves?

await BotGatewayManager.init(...)
The init of this type is async. Why is that the case and could we make it synchronous instead?

2 out of the 4 BotGatewayManager initializers, which are the ones users will use the most, initialize a DefaultDiscordClient underneath. The DefaultDiscordClient needs to acquire a shared cache, for which it'll call await ClientCacheStorage.shared.cache(for:), and that's where the async comes from. It realistically doesn't take any significant amount of time, and is just used for thread-safety reasons.
Originally i didn't like the idea to turn all those initializers to async as well, and the ClientCacheStorage used to use locking mechanisms to ensure thread-safety. I later moved ClientCacheStorage to be an actor because i came to conclusion the async inits aren't that important.

await bot.makeEventsStream()
Can we spell this as bot.events instead? Why do we need a factory method?

About spelling it as something simpler like bot.events, i didn't do that just to convey that this is not a trivial operation. It is rather light now that I'm taking a look at the implementation though, and i think i could deprecate the current function and provide an events computed property that returns an AsyncStream instead.

4

Sorry for the late reply.

Yeah normally packages don't include test abstractions because users can always do that in their application code and maybe they want a slightly different abstraction. Adding methods that such protocols is always a bit awkward since you have to provide a default implementation which often can only be fatalError.

Yeah, I think that would be better and follow what the rest of the ecosystem does.

That's interesting. Why don't we inject the cache into the client? Also what is in the shared cache between the clients?

I think having a property events looks nicer; however, you really don't want to return AsyncStream here but rather implement your own AsyncSequence which you can back by an AsyncStream. Having your own sequence allows you to swap implementations out later on. Also you probably only want to let this events property only be called once otherwise you have to think about what happens if there are multiple consumers.

1 Like
5

Yeah normally packages don't include test abstractions

Hmm can't say I'm 100% convinced, but i'll take your words for it and keep it in mind for the next major version.

[Unrelated Note Alert] Generally speaking for the next major version, as of now i only plan on a few cleanups which might result in some API breakages but won't change the shape of the library much.

Stuff like cleaning up the DefaultDiscordClient with a ClientTransport, removing those abstractions like you said, changing Snowflakes' underlying value to be of UInt64 type, are the most major changes. That release won't be coming too soon as i want to first support Voice as well as provide some kind of convenient Slash Command APIs like swift-arguments-parser or possibly with macros. The Slash Command stuff can be a game changer and be a distinguishing factor when compared to any libraries of any other languages.

That's interesting. Why don't we inject the cache into the client? Also what is in the shared cache between the clients?

The shared cache is acquired here.

What does it do? If DiscordClient.configuration.cachingBehavior allows caching a request, the ClientCache will be used as the store. Note that this behavior (response-caching) is disabled by default.

An async call is inevitable if the cache is needed, unless the cache is moved to locking mechanisms.
Maybe i could somehow provide a default init that does not do caching and can identify that so it can dodge the acquire-cache call. Currently the DefaultDiscordClient inits don't try to identify if a client-cache isn't needed, so they try to acquire a cache anyway, thus the async call.

you really don't want to return AsyncStream here but rather implement your own AsyncSequence which you can back by an AsyncStream .

I'll keep that in mind. I need to do some more digging until i can say i'm confident about my knowledge about async sequences.

Also you probably only want to let this events property only be called once otherwise you have to think about what happens if there are multiple consumers.

The library is built around having the possibility for having multiple "listeners" of gateway events. Removing that possibility can result in a decent amount of inconvenience for users from what i can think right now, because currently other types such as DiscordCache also need to somehow receive all the events separately so they can perform their own work on them.
How it all works right now is that when you do 'bot.makeEventsStream()', the GatewayManager will create a new async stream, store the new continuation in the array of continuations, return the async stream, then yield new values as they come.

6

Hello everyone!

The SSWG has decided to accept this proposal into Sandbox maturity level! :tada:

The project fulfils all requirements of the Sandbox level, including multiple maintainers and an active stream of development, good CI and adoption of concurrency features.

We'll add the project to the list of SSWG endorsed projects shortly.

5 Likes