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.

Sign up for GitHub

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

feat(core): specify event listener API #14735

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf90c23
feat(core): add event listener API
aaronc Jan 23, 2023
82e6f5f
add test
aaronc Jan 23, 2023
14a8225
simplify API
aaronc Jan 23, 2023
a4b69fd
Merge branch 'main' into aaronc/core-api-event-listeners
aaronc Jan 23, 2023
25bb942
Merge branch 'main' into aaronc/core-api-event-listeners
aaronc Jan 24, 2023
d8669fd
Merge branch 'main' into aaronc/core-api-event-listeners
aaronc Jan 24, 2023
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
32 changes: 32 additions & 0 deletions core/appmodule/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
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 •
edited
Loading

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. 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)
Copy link
Contributor

@testinginprod testinginprod Jan 23, 2023 •
edited
Loading

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

}
17 changes: 17 additions & 0 deletions core/appmodule/event_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package appmodule

import (
"context"
"reflect"
"testing"

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

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