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

Hanami::Events to implement Pub/Sub #222

Closed
wants to merge 1 commit into from

Conversation

@smakagon
Copy link

@smakagon smakagon commented Jul 24, 2017

Implementation of Hanami::Events using Wisper gem as a backend.

Implements the API provided in this PR: #160

Usage

Subscribe With A Proc

require 'hanami/events'

Hanami::Events.subscribe('user_signed_up') do |event|
  puts "New signup: #{event[:email]}"
end

Hanami::Events.broadcast('user_signed_up', id: 1, email: "user@hanamirb.org")

# => "New signup: user@hanamirb.org

Subscribe With An object

require 'hanami/events'

class WelcomeMailer
  def call(event)
    puts "New welcome message for: #{event[:email]}"
  end
end

Hanami::Events.subscribe('user_signed_up', WelcomeMailer.new)
Hanami::Events.broadcast('user_signed_up', id: 1, email: "user@hanamirb.org")

# => "New welcome message for: user@hanamirb.org

Include Mixin

require 'hanami/events'

class Signup
  include Hanami::Events

  def call(params)
    broadcast('user_signed_up', params)
  end
end

Hanami::Events.subscribe('user_signed_up') do |event|
  puts "New signup: #{event[:email]}"
end

Signup.new.call(email: "user@hanamirb.org")
# => "New signup: user@hanamirb.org
@smakagon smakagon force-pushed the smakagon:hanami_events branch 3 times, most recently from 77282de to e97bf59 Jul 25, 2017
@@ -19,6 +19,7 @@ Gem::Specification.new do |spec|
spec.required_ruby_version = '>= 2.3.0'

spec.add_dependency 'transproc', '~> 1.0'
spec.add_dependency 'wisper', '~> 2.0.0'

This comment has been minimized.

@davydovanton

davydovanton Jul 25, 2017
Member

just a question: could we drop wisper dependency? Or make it not require dependency for hanami-utils?

This comment has been minimized.

@smakagon

smakagon Jul 25, 2017
Author

That's a good question. I don't like the idea of adding more dependencies than needed, but idea of using Wisper was described here: https://discourse.hanamirb.org/t/hanami-2-0-ideas/306/9
That's why I thought that it's something we want to use for this case.

This comment has been minimized.

@cllns

cllns Jul 25, 2017
Member

I think what @davydovanton was saying is that we can make wisper an optional dependency for Hanami::Utils, by only requiring it for Hanami::Events, not all of Hanami::Utils. Doing something like:

if defined?(Wisper)
  require 'hanami/events/backends/wisper' # or something...
else
  raise "You must require `wisper` in your Gemfile in order to use `Hanami::Events`"
end

This would also let us add different backends later, which could be useful.

This comment has been minimized.

@smakagon

smakagon Jul 25, 2017
Author

Thanks. I like this idea. So this way we can get rid from hard-coded dependency and let clients decide which backend they want to use.

@davydovanton
Copy link
Member

@davydovanton davydovanton commented Jul 25, 2017

Hey, I think you need to check this PR. @jodosha worked on it and you can inspire his work I think :)
#160

@smakagon
Copy link
Author

@smakagon smakagon commented Jul 25, 2017

@davydovanton thanks for the feedback, I mentioned #160 PR in my description to current one. My intent was to implement the same API for Events as Luca did in #160 but with Wisper as a backend.

@smakagon smakagon force-pushed the smakagon:hanami_events branch from e97bf59 to a3a0274 Jul 26, 2017
@smakagon
Copy link
Author

@smakagon smakagon commented Jul 26, 2017

@davydovanton @cllns @jodosha I've updated PR:

  • removed wisper from gem dependencies
  • extracted Backends::Wisper into separate file

Let me know what you think.
Btw, CI is red because it can't find wisper gem. Should I add wisper to Gemfile under if ENV['TRAVIS'] section?

@davydovanton
Copy link
Member

@davydovanton davydovanton commented Jul 26, 2017

@smakagon you can add wisper as a development dependency :)

@smakagon smakagon force-pushed the smakagon:hanami_events branch from a3a0274 to d404437 Jul 26, 2017
@smakagon
Copy link
Author

@smakagon smakagon commented Jul 26, 2017

Done.

Copy link
Member

@jodosha jodosha left a comment

@smakagon Thanks for this PR.

We're missing an "isolation" spec to make sure that the absence of wisper will raise an error. For this purpose, please have a look at: https://github.com/hanami/utils/blob/master/spec/isolation/hanami/utils/json/json_spec.rb

Hint: you should move wisper from dev deps in gemspec to a separated Gemfile group named :events. That will allow to selectively load that group via Bundler.


What we should avoid, once again, is: ..global state! 🎉

This design uses class methods which registers globally the subscriptions.

What we should aim to is:

events = Hanami::Events.new
events.subscribe(:signup, mailer)
events.broadcast(:signup, user: 1)
@@ -2,6 +2,7 @@
#
# @since 0.1.0
module Hanami
require 'hanami/events'

This comment has been minimized.

@jodosha

jodosha Jul 26, 2017
Member

@smakagon This shouldn't be required by default.

begin
require 'hanami/events/backends/wisper'
rescue LoadError
raise "Couldn't find `wisper` gem. Please, add `gem 'wisper', '2.0.0'` to Gemfile in order to use `Hanami::Events`"

This comment has been minimized.

@cllns

cllns Jul 26, 2017
Member

Maybe we should do ~> 2.0, assuming wisper uses SemVer? rather than locking them down to one very specific version

@davydovanton
Copy link
Member

@davydovanton davydovanton commented Jul 29, 2017

Hello, thanks for PR!

After discussion with @jodosha, we decided that this library should look like another gem.
Also, we think that hanami-events should use different adapters and be without a global state.

Why different adapters?

I think hanami events should be a library for building Event-Driven Architecture. In this case, a library should work with different hanami project instances:

screenshot 2017-07-29 15 27 13

In this case, we can provide different transport layers for a event storage like:

  • Memory
  • Postgres
  • Kafka
  • Kinesis
  • etc

And that's why we need to replace wispers gem as an adapter.

From which hanami events should be?

I think that we need to split all logic to 3 different parts:

  1. Broadcaster
  2. Subscriber
  3. Routing

All these parts should be without a global state. And here my ideas about API for all this.

Broadcaster

events = Hanami::Events.new(:adapter)
events.broadcast('user.created', user: user)

Subscriber

kafka_events = Hanami::Events.new(:kafka)
memory_events = Hanami::Events.new(:memory)

memory_events.subscribe('user.signup', Mailer)

# or in class
class Signup
  inclide Hanami::Events::Subscriber
  subscribe_to memory_events, 'user.signup'
  subscribe_to kafka_events, 'user.signup'

  def call(payload)
    # ...
  end
end

Also, I think we should have an ability for subscribing to all events:

kafka_events = Hanami::Events.new(:kafka)

kafka_events.listen do |event|
  event.name    # => 'user.signup'
  event.payload # => { ... }
end

Routing

Routing should route all events and call specific code for each one. It will be helpful for manage all events in one place (look like HTTP routers):

class WebHandler < Hanami::Events::Handler
  on('user.created') { |payload| UserRepository.new.create(payload) }
  on('user.updated') { |payload| payload }
end

class AnaliticHandler < Hanami::Events::Handler
  on('*', AnaliticSender)
  on('user.*') { |payload| payload }
  on('user.signup') { |payload| payload }
end

class NotificationHandler < Hanami::Events::Handler
  on('user.created') { |payload| payload }
  on('post.created') { |payload| payload }
end

class EventRouting < Hanami::Events::Routing
  mount UserHandler, Hanami.app?(:user_events) # load specific handlers for instance
  mount AnaliticHandler
  mount NotificationHandler
end

events = Hanami::Events.new(:adapter)
events.listen { |event| EventRouter.new.resolve(event) }

I think we can try to implement proof of concept for this gem. WDYT?

@smakagon
Copy link
Author

@smakagon smakagon commented Jul 29, 2017

@davydovanton @jodosha looks really good. Thanks for a detailed description of idea. Let's implement it as a separate gem with support for different adapters. I'm in :)
Should I close this PR?

@jodosha
Copy link
Member

@jodosha jodosha commented Jul 31, 2017

@davydovanton Thanks for the detailed explanation. However, I'm not convinced with a few points above:

  1. Multiple subscriptions for a single object (eg. subscribe_to memory_events/kafka_events)
  2. Class subscribers (eg. memory_events.subscribe('user.signup', Mailer))
  3. Routing

Can we think of a simplified proof of concept, without these features? It's easier to incrementally build on top of a set of core features.

We can think to start with memory and redis adapters. They are easy to reason about, to develop, and deploy.


@davydovanton Can you update the Hanami 2.0 ideas thread with your proposal? https://discourse.hanamirb.org/t/hanami-2-0-ideas/306/47 Thanks!

@jodosha jodosha self-assigned this Jul 31, 2017
@jodosha
Copy link
Member

@jodosha jodosha commented Jul 31, 2017

I'm closing this in favor of https://github.com/hanami/events

@jodosha jodosha closed this Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.