GatewayEventInterceptor customization

[ByteAlex]

On-Top of #390

Added a nullable "extraContext" field to the Kord main class. Kord dispatches events on with the scope & context of the main class. For my use case I need to inject a context on a per-event basis. To achieve this the context builder is called on every event (if not null) and appends its context to the main classes context.

[BartArys]

The signature of both interfaces differs only by an additional serialize method, which currently only fails with a "TODO" call.

If we're not going to implement the function we shouldn't implement the interface either. Events will become fully de/serializable in the future, but I understand if you need a solution now for your issue.

If you don't care about serializing events, and control the container class that has the event property, you can write your own serializer that delegates to ours.

object MyEventSerializer : KSerializer<dev.kord.gateway.Event> {

    override val descriptor: SerialDescriptor
        get() = dev.kord.gateway.Event.descriptor

    override fun deserialize(decoder: Decoder): dev.kord.gateway.Event {
        return dev.kord.gateway.Event.Companion.deserialize(decoder)!!
    }

    override fun serialize(encoder: Encoder, value: dev.kord.gateway.Event) {
        error("not implemented")
    }

}

@Serializable
data class MyCustomContainer(
    @Serializable(with = MyEventSerializer::class)
    val event: dev.kord.gateway.Event
)

[ByteAlex]

This one is an acceptable solution for me. I just wanted to stay as close as possible to the Kord model, since diverging too much will give me a huge headache with the library moving forward. Though delegating the serializer to an existing serializer function in Kord is a fair solution. That works for me.

How's it looking for the extraContext field on the Kord instance, to get an CoroutineContext attached to an event which is about to be dispatched?

[ByteAlex]

I dropped the commit for KSerializer from the commit history.

[ByteAlex]

Renamed the PR title and changed implementations

[ByteAlex]

I do not have a PoC for this implementation yet, but would the general design approach work for you guys @BartArys @HopeBaron ?
Just making sure, this gets an OK before I pour more time into a dead end :p

[ByteAlex]

When having a short check on whether this is feasible, I noticed the coroutineContext field needs to be exposed via constructor.
Other than that, this approach seems to work for me, even though it'll be a lot of writing since I need to mimic the entire Kord gateway.Event -> core.Event mapping.

[BartArys]

Giving events a dedicated coroutine scope is the way forward, but not through directly adopting Kord's coroutine context.

Events should have their own lifecycle inherited from Kord, which you can achieve by wrapping Kord's job in a SupervisorJob. To prevent user implementation mistakes we should also accept a CoroutineScope instead of a context as per kotlinx.coroutines documentation.

class ReactionRemoveEmojiEvent(
    val data: ReactionRemoveEmojiData,
    override val kord: Kord,
    override val shard: Int,
    override val supplier: EntitySupplier = kord.defaultSupplier,
    coroutineScope: CoroutineScope = CoroutineScope(kord.coroutineContext + SupervisorJob(kord.coroutineContext.job))
) : Event, Strategizable, CoroutineScope by coroutineScope

[BartArys]

Other than that, this approach seems to work for me, even though it'll be a lot of writing since I need to mimic the entire Kord gateway.Event -> core.Event mapping.

I'm willing to accept a (GatewayEvent, Kord) -> CoroutineScope property in a DefaultGatewayInterceptorBuilder or whatever we'll end up with. Unlike putting it in Kord this fits with the responsibilities of the code.

I'm not really sure what else people would want to configure for the interceptor though, but this should give us enough flexibility to move forward.

[ByteAlex]

Events should have their own lifecycle inherited from Kord, which you can achieve by wrapping Kord's job in a SupervisorJob. To prevent user implementation mistakes we should also accept a CoroutineScope instead of a context as per kotlinx.coroutines documentation.

I just adapted how coroutines are launched in the current version of Kord. If you want me to change that with this PR, that is fine by me. I just wanted to clarify that this change tried to maintain the current behaviour of Kord as far as possible.

I'm not really sure what else people would want to configure for the interceptor though, but this should give us enough flexibility to move forward.

Eventually configure custom events or custom event mappings. I'd have it abused to pull in the "old" guild-data for the GuildUpdateEvent for example, since I can not pull it from cache, I need to have it in my event data, simply because it's already deleted on the cache when the event reaches the Kord instance.

[ByteAlex]

I think I resolved all of your comments by now @BartArys. Could you please review again?

[HopeBaron]

I'm willing to accept a (GatewayEvent, Kord) -> CoroutineScope property in a DefaultGatewayInterceptorBuilder or whatever we'll end up with. Unlike putting it in Kord this fits with the responsibilities of the code.

I find that the logic of intercepting something is up to the user, and allowing the user to implement it themselves sounds more flexible

[ByteAlex]

I find that the logic of intercepting something is up to the user, and allowing the user to implement it themselves sounds more flexible

I built it to be flexible in multiple regards, you can overwrite the "handle" method for a "simple" interceptor, or you could also overwrite the "start" method for a more complex interceptor (i.e. custom downstream events).

For the start the (GatewayEvent, Kord) -> CoroutineScope in the DefaultGatewayInterceptor comes in quite handy tho, because it saves me the entire gateway-event -> core-event mapping and I can just hit it with a coroutine scope provider.

Though it might be useful to open up the current "Listeners", so an eventual outside interceptor can extend and overwrite what they need. Would that be an acceptable thing?

[HopeBaron]

I'll have to wait for Bart to see which approach should we take as both seem fine to me.