looplab / eventhorizon

Event Sourcing for Go!
Apache License 2.0
1.6k stars 196 forks source link

[commandhandler] Add back ReflectCommandHandler #146

Open chriskolenko opened 7 years ago

chriskolenko commented 7 years ago

Instead of having HandleCommand(ctx context.Context, cmd eventhorizon.Command) ApplyEvent(ctx context.Context, event eventhorizon.Event) ETC.

Could we use reflection to find HandleCommandXXX(ctx context.Context, cmd custom.Command) And invoke that method instead of the HandleCommand?

Since we have a registry of everything we could do it on start up and keep a cache around of methods?

maxekman commented 7 years ago

That was actually how it worked in the very beginning. Take a look at https://github.com/looplab/eventhorizon/commit/0a6fc3435ba4a8287642ec439a0857045b3be2a3.

It could be nice to add back a CommandHandler that does that and I'm happy to accept a PR if you want to give it a go.

maxekman commented 7 years ago

We opted to use the switch statements (in a current project) to get better compile time checking and to avoid the possible performance issues with using the reflect package.

chriskolenko commented 7 years ago

I should have wrote back yesterday apologies.

I've been pondering over this for a day now. Thinking about the correct design.

I believe I'm going to tackle it a little differently. Create some sort of Registration process.

RegistryBuilder.
  New().
  AddAggregate(&Name1{}). // Maybe use the factory here
  AddAggregateFactory(func(UUID) { return &Name2{} }).
  AddEventHandler(eventHandler1).
  EventBus(eventBus).
  CommandBus(commandBus).
  Build()

AddAggregate and AddEventHandler will use reflection to build up some sort of Handlers cache. This will also call all the internal Register functions needed.

Then I'll just create a Base Class that uses the Context to find the Handlers cache and executes it.

This will keep the code seperate from the core library. Could included from another Package etc. Less breaking changes.

Thoughts?

maxekman commented 7 years ago

Sounds like a neat way to go with the setup! I have not yet put much work into making the setup more semantic and I definitely think there are too many moving parts in the init process as it is now.