feat(core): specify event listener API #14735
Conversation
github-prbot
requested review from
a team,
kocubinski and
tac0turtle
and removed request for
a team
| type HasEventListeners interface { | ||
| AppModule | ||
|
|
||
| // RegisterEventListeners registers the module's events listeners. | ||
| RegisterEventListeners(registrar *EventListenerRegistrar) | ||
| } |
There was a problem hiding this comment.
I have a question on why to expose EventListenerRegistrar in core, shouldn't that be runtime?
The core interface could simply be:
| type HasEventListeners interface { | |
| AppModule | |
| // RegisterEventListeners registers the module's events listeners. | |
| RegisterEventListeners(registrar *EventListenerRegistrar) | |
| } | |
| type HasEventListeners interface { | |
| AppModule | |
| RegisterEventListeners() []func(context.Context, protoiface.MessageV1) | |
| RegisterEventInterceptors() []func(context.Context, protoiface.MessageV1) error | |
| } |
There was a problem hiding this comment.
It's just a more ergonomic API this way I think
There was a problem hiding this comment.
EventListenerRegistrar is just a dummy struct to make using generics easy
aaronc
changed the title
| // RegisterEventListener registers an event listener for event type E. If a non-nil error is returned by the listener, | ||
| // it will cause the process which emitted the event to fail. | ||
| func RegisterEventListener[E protoiface.MessageV1](registrar *EventListenerRegistrar, listener func(context.Context, E) error) { | ||
| registrar.listeners = append(registrar.listeners, listener) |
There was a problem hiding this comment.
why not this instead of using []any.
| registrar.listeners = append(registrar.listeners, listener) | |
| listenerF := func(ctx context.Context, iMsg protoiface.MessageV1) error { | |
| concrete, ok := iMsg.(E) | |
| if !ok { return fmt.Errorf("...") | |
| return listener(ctx, concrete) | |
| } | |
| registrar.listeners = append(registrar.listeners, listenerF) |
There was a problem hiding this comment.
Because the caller will be doing a type check anyway. I think this would just add extra type checking. Also in core we don't want to add any implementation details. This should be defer all those details to runtime
|
|
||
| // EventListenerRegistrar allows registering event listeners. | ||
| type EventListenerRegistrar struct { | ||
| listeners []any |
There was a problem hiding this comment.
| listeners []any | |
| listeners []func(ctx context.Context, iMsg protoiface.MessageV1) error |
|
Are we okay to merge this? I think this should have a standalone ADR by the way. I can work on that after #12972. |
|
I'm taking the 👍 as a yes |
Description
Ref #14683
This adds an API for modules to register event listeners in core.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change