Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): specify event listener API #14735

Merged
merged 6 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions core/appmodule/event.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package appmodule

import (
"context"

"google.golang.org/protobuf/runtime/protoiface"
)

// HasEventListeners is the extension interface that modules should implement to register
// event listeners.
type HasEventListeners interface {
AppModule

// RegisterEventListeners registers the module's events listeners.
RegisterEventListeners(registrar *EventListenerRegistrar)
}
Comment on lines +11 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question on why to expose EventListenerRegistrar in core, shouldn't that be runtime?

The core interface could simply be:

Suggested change
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
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a more ergonomic API this way I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EventListenerRegistrar is just a dummy struct to make using generics easy


// EventListenerRegistrar allows registering event listeners.
type EventListenerRegistrar struct {
listeners []any
Copy link
Contributor

@testinginprod testinginprod Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
listeners []any
listeners []func(ctx context.Context, iMsg protoiface.MessageV1) error

}

// GetListeners gets the event listeners that have been registered
func (e *EventListenerRegistrar) GetListeners() []any {
return e.listeners
}
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved

// RegisterEventListener registers an event listener for event type E.
func RegisterEventListener[E protoiface.MessageV1](registrar *EventListenerRegistrar, listener func(context.Context, E)) {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
registrar.listeners = append(registrar.listeners, listener)
Copy link
Contributor

@testinginprod testinginprod Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not this instead of using []any.

Suggested change
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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// RegisterEventInterceptor registers an event interceptor for event type E. Event interceptors can return errors
// to cause the process which emitted the event to fail.
func RegisterEventInterceptor[E protoiface.MessageV1](registrar *EventListenerRegistrar, interceptor func(context.Context, E) error) {
aaronc marked this conversation as resolved.
Show resolved Hide resolved
registrar.listeners = append(registrar.listeners, interceptor)
}
22 changes: 22 additions & 0 deletions core/appmodule/event_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package appmodule

import (
"context"
"reflect"
"testing"

"github.com/stretchr/testify/require"
"google.golang.org/protobuf/types/known/structpb"
"google.golang.org/protobuf/types/known/timestamppb"
)

func TestEventListenerRegistrar(t *testing.T) {
registrar := &EventListenerRegistrar{}
RegisterEventListener(registrar, func(ctx context.Context, dummy *timestamppb.Timestamp) {})
RegisterEventInterceptor(registrar, func(ctx context.Context, dummy *structpb.Struct) error {
return nil
})
require.Len(t, registrar.listeners, 2)
require.Equal(t, reflect.Func, reflect.TypeOf(registrar.listeners[0]).Kind())
require.Equal(t, reflect.Func, reflect.TypeOf(registrar.listeners[1]).Kind())
}