WIP: Discussion - Expose Event Name from Packet

[mmellison]

Problem: We listen to multiple Vici events, but currently do not have a way to easily determine which type of event was returned by session.NextEvent(). Currently it only returns the underlying message.

Solution: Return the event name with the message (as sent from the server). This allows us to easily switch-case on the returned event name.

---

This was just a quick diff I threw together to have a working proof of concept for discussion. The actual interface can be different, since this of course breaks the current API.

I suppose if you wanted strict backwards compatibility you could have a session.NextEventWithName() and just have a third return value, but switching to a struct now will allow us to add new fields later without breaking the API.

I'll leave the design decision up to you :)

[enr0n]

+1 on the intention.

I think I'm OK with breaking the API here if necessary (in the README I note that may happen before v1.0.0). I'll work on the patch and figure out how the API would look.

I've actually been thinking about the event listener API recently, wondering if there's any big room for improvement. Besides the issue you raised here, how do you find working with events? Does the

s.Listen(ctx, "event1", "event2")

for {
        e, err := s.NextEvent()
}

pattern make sense to you? If the API is going to change anyways, is there anything else you would want added/changed?

[mmellison]

Couple things I've thought about:

• It would be nice to be able to dynamically subscribe and unsubscribe from events without having to stop the listener, since during this time we could miss messages. With the current API we have to stop listening to all current events and re-subscribe with a new list.

• It feels a little awkward to pass a context into Listen. Sometime I need to only stop the current NextEvent call without stopping the entire listener. I think a context should be passed into each "NextEvent" function instead so that I can do things like:

ctx := context.WithDeadline(...)
s.NextEvent(ctx)

Perhaps something like this:

ctx := context.TODO()

// Example 1
s.Subscribe("event1")   // Underlying listener starts
for err == nil {
    event, err = s.NextEvent(ctx)
}
s.Unsubscribe("event1") // Underlying listener stops since we unsubscribed from our last event

// Example 2
s.Subscribe("event1", "event2") // Underlying listener starts
s.Subscribe("event3")
s.Unsubscribe("event1")  // Underlying listener stays alive since we're still subscribed to event2 and event3
s.Close() // Implicitly unsubscribe from all existing events and stop the listener.

Suppose you could also have an s.UnsubscribeAll() if the user doesnt want to close the session but unsubscribe from all current events.

[enr0n]

Thanks for the suggestions here. I opened #18 and #19 to track the two ideas.