v3: What should we do with interfaces?

[foxbot]

In v3, entities will be stripped of most of their function, and will be wrapping over Wumpus.Net models, rather than Discord.Net interfaces.

Should we still keep our entities dependent on a core interface? This has the advantage of making the library easier to mock in downstream projects, but few if any projects actually use the interfaces as intended, and instead couple their projects directly to socket entities.

Interfaces also make our codebase something of a minefield to work with, especially when navigating API changes on Discord's end - any new feature should constitute a major version bump, since we should be updating our interfaces in response.

[foxbot]

I've drawn out some very helpful graphs which demonstrate my proposal for how we restructure interfaces.

As of the v1 design, Socket and REST classes have a strict dependence on the Core interfaces, and the Socket client has a strict dependence on the REST client - under the hood, the Socket client is actually composed of the REST client.

If we draw the Wumpus projects in on top of the Discord.Net projects, we get a very ugly diagram where Discord.Net's internal dependence fights over its dependence on Wumpus.

Note that in both of these cases, the consumer is fully aware of the interfaces that the Discord entities depend on, but it doesn't really use them, except for in very rare cases (GetMessageAsync is about the only one).

Drawing the diagram from scratch yields us a much prettier dependency tree, with a very vague "common interface" section in the middle.

I think at this point the library should be specific about what kind of design we should be encouraging; will "common interfaces" be the preferred channel of using the library?

If we're going to prefer using interfaces for everything, all public methods should return an interface, and we shouldn't be making a distinction between socket/rest, except for on the clients themselves (events and constructors are pretty much the only reason to need a concrete class). Bear in mind that this means consumers will be unable to use navigation properties, and will be forced to await them - ValueTask means this shouldn't be much of an issue though.

Discord's API is also relatively volatile - the quarterly API updates will almost guarantee that we will need to push a major version shortly after. The previous nature of Discord's API makes this problematic, since they tend to iterate on features again after release (the ever expanding activities API comes to mind here).

Pushing concrete classes almost everywhere seems acceptable to me; except that the "common interface" library would need to grow pretty large for the few places we would need it. We could maybe limit it on purpose, sacrificing functionality in favor of cleanliness...

The common "IMessage" interface, for example, could sacrifice navigation methods in favor of keeping only the ID fields. This would save us from needing to stub out a guild interface, and then role interfaces, keeping duplicate properties...

The same idea could apply to an "IGuildMember" in the GET /guilds/:id/members/:id wrapper...

The latter option seems most reasonable to me, opinions?

[Yucked]

Can we completely remove public interfaces and just stick with classes and go with option 3?

[foxbot]

No, there are a number of instances where the library requires public interfaces to remain high-level.

Channels being split into more specific types (guild, text, DM) require common interfaces; alternatively we could regress to a single channel type as we saw in 0.9, but then we would be removing compile-time sanity checks to prevent bulk deletes in DMs, for example.

[AntiTcb]

Carrying conversation over from Discord, I'm in favour of keeping the interfaces, and we should really be less averse to having more frequent major updates. Discord's a volatile system, and we're using semver, that's just going to be a fact/consequence of life when you mix the two together.

[FiniteReality]

I think making the backing types internal and exposing interfaces would be a good approach. We can (ab)use ValueTask as well as unify a lot of the API, e.g. exposing caching options where possible. It would also simplify consumption, as there are only a single set of types to develop against. In an ideal world, a user shouldn't need to know whether an entity came from the REST API or the gateway, because functionally we can expose more or less the same behaviour on them.

Of course, exclusions such as voice exist, but I think a simple solution would be to instead expose more of the underlying API in this case, e.g. UpdateVoiceStateAsync() on IAudioChannel or whatever we decide to name it. As it stands right now, connecting to a voice channel coerces what's not really a request/response system into a request and response, and that causes a lot of awkwardness for consumers.

As for events, I don't mind Task-returning events, but they must be detached from the gateway and queued properly if we are to keep it.

[Joe4evr]

I'm in favour of keeping the interfaces, and we should really be less averse to having more frequent major updates.

How many users do we have that are re-implementing the interfaces anyway? Because if they are, they're doing it wrong. The interfaces only exist for consumption/API sanity and are only implemented by us, see also the language design notes for InternalImplementationOnly:

They are not intended as extension points, we merely made them interfaces to allow multiple base types. On the contrary we would like to be able to add members to these types in the future, as the Roslyn Discord API evolves.

[JustNrik]

I agree with Joe and Yucked, also, current d.net design is a real design smell, interfaces are being heavily misused. Such senseless interfaces as IGuild, IChannel, IUser and their children should be deleted or become internal as they just make no sense. They are doing the job of what an abstract class is meant to do semantically, whereas interfaces like IDeleteable, IMentionable, etc... Are properly implemented and follow proper design semantics. In my opinion, and considering there aren't use case where exposing interfaces that do the job of an abstract class is actually useful, I suggest to stick on wumpus entities for the sake of dnet 3 design and stop the massive polymorphims abuse.

[Joe4evr]

stop the massive polymorphims abuse.

This is not at all what I said. I think the polymorphism is actually good because it provides compile-time safety for what can and can't be called on a given type. My opinion boils down to the following:

• Keep the interfaces and hierarchy (but maybe re-think a few things)
• Hide the concrete backing types if at all possible (ValueTask<T> allows (near-)0-cost async-over-sync)
• Tell semver to go take a hike when we have to add something to support Discord's latest new thing (users shouldn't be making their own implementations; they need only consume the interfaces, so we shouldn't consider pure addition a breaking change)

That's how it works in Roslyn's ISymbol API, and it's working surprisingly well.

[JakenVeina]

Can we completely remove public interfaces and just stick with classes and go with option 3?

Unless you're going to make all classes unsealed and all members virtual, this would be a rather large step backwards. Without interfaces, your API is not mockable, making it impossible for consumers to write unit/system tests.

All of the biggest Discord.NET hurdles we've had to overcome in https://github.com/discord-csharp/MODiX have basically been interface-related, so IMO there's definitely room for improvements.

The most significant issue, for me, has been that the interface API has quite a few gaps in functionality, meaning that we have features that cannot be tested without writing a layer of wrapper interfaces around the API. The one that occurs to me, at the moment, involves the BaseSocketClient.ReactionAdded and BaseSocketClient.ReactionRemoved events. The underlying socket message from Discord includes a userId field for these messages, which is exposed through the event on SocketReaction. However, this property is not present on IReaction which is the only interface that SocketReaction implements, and since the only constructor for SocketReaction is internal, it's impossible to mock out an occurrance of this event in testing, if the unit under test requires use of this field (in our specific case, we use reactions as a kind of pseudo-button for users to interact with MODiX, and want to ignore any reactions manipulated by MODiX itself, or other bots).

Another issue that's cropped up in a few different ways is that some interface implementations do not implement all of the required members. For example, when the PRESENCE_UPDATE event occurs, it's routed through the API as BaseSocketClient.GuildMemberUpdated and the message data is exposed as SocketGuildUser despite the fact that the message itself is not guaranteed to contain any guild or user information beyond userId. This ultimately results in objects that implement IGuildUser that have a .Guild property of null and will throw NullReferenceException if you try and accessGuildId. IMO, if an interface defines something, implementers had better implement it. If it can't be fully implemented, another interface or class should be chosen to represent the data.

Another issue we've had that could be solved with having a wider variety of interfaces is that we actually use both DiscordSocketClient and DiscordRestClient in the same application. RestDiscordClient is only used in one specific case where we want to try and lookup a user that isn't actually in our guild (for the sake of pre-emptive banning). Since DiscordSocketClient only pulls user data from the local cache, we have to use a DiscordSocketClient for that query, and since our DiscordSocketClient is already registered as IDiscordClient in the DI container, DiscordRestClient has to be registered as DiscordRestClient, making it impossible to test this bit of functionality.

Don't mean to hate or nit-pick, just want to provide some insight into what the frustrating points are for the API, today.

[Joe4evr]

Unless you're going to make all classes unsealed and all members virtual, this would be a rather large step backwards. Without interfaces, your API is not mockable, making it impossible for consumers to write unit/system tests.

It's still very badly mockable, though. I'd rather see something more akin to unit tests running against a local (more basic) mock of Discord's API, that can be configured to simulate a number of guilds, channels and users, with a nice little method that simulates one such user sending a message. This makes it such that mock implementations of all the entities isn't needed.

Another issue we've had that could be solved with having a wider variety of interfaces is that we actually use both DiscordSocketClient and DiscordRestClient in the same application. RestDiscordClient is only used in one specific case where we want to try and lookup a user that isn't actually in our guild (for the sake of pre-emptive banning). Since DiscordSocketClient only pulls user data from the local cache, we have to use a DiscordSocketClient for that query, and since our DiscordSocketClient is already registered as IDiscordClient in the DI container, DiscordRestClient has to be registered as DiscordRestClient, making it impossible to test this bit of functionality.

I think what we're gonna do is only offer one client type that just does WebSocket, with either an implicit or explicit fallback to Rest for requests where it wasn't found the first time.

[FiniteReality]

I think what we're gonna do is only offer one client type that just does WebSocket, with either an implicit or explicit fallback to Rest for requests where it wasn't found the first time.

I don't know if it's been decided or not, but I do think this approach would be best for Discord.Net in the long term. Simplifying our API consumption points and focusing only on bot developers makes a lot of sense. Wumpus.Net's REST API isn't particularly hard to consume and we should direct people who need OAuth/etc that way.

[Joe4evr]

I don't know if it's been decided or not,

Fox's initial stub is simply named DiscordClient and exposes a REST client property, so I was assuming this is the direction he intends to go in (and I think that's fair).

[Yucked]

Mocking stuff seems more like an edge-case to me. Majority of us (everyday users) don't even care about unit tests or mocking. We just want a library that doesn't have 5 interfaces of x thing: IChannel, IDMChannel, IPrivateChannel, IGroupChannel, ITextChannel, IMessageChannel, etc.

My point simply boils down to: simplify library for an average user that doesn't get confused with all the interfaces. Mocking stuff can be added as an extension for library.

[FiniteReality]

Mocking stuff seems more like an edge-case to me. Majority of us (everyday users) don't even care about unit tests or mocking. We just want a library that doesn't have 5 interfaces of x thing: IChannel, IDMChannel, IPrivateChannel, IGroupChannel, ITextChannel, IMessageChannel, etc.

My point simply boils down to: simply library for an average user that doesn't get confused with all the interfaces. Mocking stuff can be added as an extension for library.

These interfaces are important, and I'm pretty certain they're going to stay, simply for type safety. If anything, what will change is not having double that with exposing the Socket or Rest entities too.

[WamWooWam]

I personally have felt for a while that trying to bodge type safety into a system which is inherently non type safe and mutates and changes often isn’t the greatest idea.

Maintainability is key, and it can be a real pain to add some new features to D.Net at times when it requires modifying many interfaces, extension methods and implementations, I don’t feel this is really an asset when Discord as a platform adds new features quite regularly.

On top of this, some objects can accept properties they generally shouldn’t be able to, for example voice channels can be marked as NSFW. This isn’t and can’t easily be exposed with the current system, technically NSFW isn’t a property on voice channels, so in a type safe world, shouldn’t be in an IVoiceChannel interface, but Discord isn’t type safe in this way.

I also don’t understand the distinction between REST and Socket objects, it doesn’t really make much sense to me. On the backend the JSON wrapped here is basically the same, as are the methods used to mutate and delete them. I can’t personally see much of a point here.

The argument for unit tests? Discord as a platform isn’t particularly mockable to begin with, unusual behaviours and oddities pop up all over the place and it’s very difficult to mock all of these intricacies accurately and write appropriate tests.

Mocking is further complicated because most of D.Net’s interfaces are highly mutable, inherent to the platform it’s a wrapper for. Any change to an interface is a breaking one, they should be fixed but can’t be.

And, all this aside, frankly, casting isn’t intuitive.

TL;DR, I feel it’d benefit maintainability and usability to merge REST and Socket objects, and consolidate interfaces to something more simple and generic.

[Joe4evr]

I personally have felt for a while that trying to bodge type safety into a system which is inherently non type safe and mutates and changes often isn’t the greatest idea.

That's just, like, your opinion, man. There's not exactly a lot we can do here. I mean, it's extremely unlikely for us to convince the Discord devs to only push changes every quarter or something because you feel we can't evolve our API faster than that. If you're that concerned with the type-safety of the language getting in the way of the API evolution, then you're free to use a lib written in a dynamic language instead.

Maintainability is key, and it can be a real pain to add some new features to D.Net at times when it requires modifying many interfaces, extension methods and implementations, I don’t feel this is really an asset when Discord as a platform adds new features quite regularly.

And so, if my suggestion is agreed to, pure addition of functionality will be considered only a minor version change instead of a major, allowing us to support new Discord features pretty much as they come out.

I also don’t understand the distinction between REST and Socket objects, it doesn’t really make much sense to me. On the backend the JSON wrapped here is basically the same, as are the methods used to mutate and delete them. I can’t personally see much of a point here. [...] TL;DR, I feel it’d benefit maintainability and usability to merge REST and Socket objects, and consolidate interfaces to something more simple and generic.

Yes, achieving such consolidation is precisely why I'm arguing the 3.0 design actually hide the concrete types, and consumers only get interfaces.

[WamWooWam]

If you're that concerned with the type-safety of the language getting in the way of the API evolution, then you're free to use a lib written in a dynamic language instead.

It's less a consern about general tyesafety, more the way it's handled in D.Net*, with specific interfaces and types for things that on the API side aren't type safe, like ITextChannel alongside IMessageChannel alongside IGuildChannel alongside IChannel, or IMessage alongside IUserMessage alongside ISystemMessage.

These, on the API side anyway, aren't super distinct types, defined more by their type JSON property than the actual properties they have.

Generally I'm all for type safety in languages, but I'm not sure this specific way of doing type safety fits Discord's API well.

* I should’ve been a little more specific here initially to be fair.

[Joe4evr]

like ITextChannel alongside IMessageChannel alongside IChannel, these on the API side aren't super distinct types, defined more by their type property than the actual properties they have. [...] These, on the API side anyway, aren't super distinct types, defined more by their type JSON property than the actual properties they have.

We had that design of a single Channel type back in 0.9, which was the union of all the things different channels could do. It sucked, because the type would expose stuff that couldn't be called on every type of channel, leading to users making bad assumptions all around.

[WamWooWam]

Maybe something like 4 types, each for Voice, Text, Group and DM? Or something similar that strikes a balance between safety and simplicity?

[foxbot]

That is essentially what we already have, with the proper layers of abstraction in between. The only changes being made in 3.x are the removal of group-related abstractions, since bots don't have access to them.

An abstraction layer for messages is necessary so that services can interact with a channel, regardless of whether or not it is a DM. Having DM/Text channels be separate with no shared ancestry would require two code paths in place of one. The same philosophy applies for Guild related abstractions.