krisleech / wisper

A micro library providing Ruby objects with Publish-Subscribe capabilities
3.27k stars 151 forks source link

More flexibility for filtering events when subscribing #172

Closed lukeredpath closed 5 years ago

lukeredpath commented 5 years ago

This is something I might consider submitting a PR for in the future but I wanted to raise an issue for discussion first to see what people thought of the idea.

We have a scenario in an app I'm working on where a single model, which I'll call Widget, broadcasts events throughout it's lifecycle. We have a workflow associated with these events and we drive that workflow by having a dedicated Workflow class that subscribes to events from Widget. So far, so simple.

The issue arises where Widget can have a particular type attribute. The events broadcast are the same but how we handle those events changes depending on the type - the different types have different workflows.

The way we model this is to have a WidgetTypeAWorkflow and WidgetTypeBWorkflow listener. The issue is ensuring that each listener only receives events from Widget if it has the corresponding type. We don't want to have a guard clause in each of our event listener methods. In an ideal world, I'd like to be able to filter out events from Widget based on the type at subscription time.

Wisper only provides us with limited options for filtering out events - it does allow us to scope subscribers to particular broadcasters. Therefore, out current solution is to have two internal classes to Widget - TypeABroadcaster and TypeBBroadcaster and do a little bit of Ruby magic so what whenever we broadcast an event inside Widget, it delegates broadcasting to a concrete broadcaster object of one of these types. This allows us to scope the two workflow classes to the specific broadcaster using the existing source: option.

However, it feels like it would be nicer if we could just broadcast events without any of the above complexity and simply have our listeners specify how events should be filtered when they subscribe. It might look something like this:

class WidgetTypeEventFilter
  def initialize(expected_type)
    @expected_type = expected_type
  end

  def forward_event_to_listener?(event, sender) # API open for debate
    return sender.type == @expected_type
  end
end

Wisper.susbscribe(WidgetTypeAWorkflow.new, filter: WidgetTypeEventFilter.new('type_a'))
Wisper.susbscribe(WidgetTypeBWorkflow.new, filter: WidgetTypeEventFilter.new('type_ab))

You could have a built-in block based filter type that is initialised internally if the user passes a proc to the filter: option for convenince:

class BlockEventFilter
  def initialize(proc)
    @proc = proc
  end

  def forward_event_to_listener?(event, sender)
    @proc.call(event, sender)
  end
end

Wisper.subscribe(SomeListener.new, filter: ->(event, sender) { check_something(event) })

# also support blocks?

Wisper.subscribe(SomeListener.new) do |event, sender|
  check_something(event)
end

You could also refactor internally and make the existing source: option work as a filter:

class EventSourceFilter
  def initialize(source_type)
    @source_type = source_type
  end

  def forward_event_to_listener?(_event, sender)
    return sender.is_a?(@source_type)
  end
end

Any thoughts on this?

krisleech commented 5 years ago

I don't have time to look at your proposal right now but to be honest it is unlikely that I would except such as feature as the complexity added to the codebase would likely outweigh the gains. But open to discussion.

It seems to be the same problem as handling different versions of an event. Say we have a version attribute in the event payload and it is bumped as per semver when the payload changes.

It might look like:

class MyListener::V1
  def on_event(event)
    return unless event.fetch(:version) == 1
     # ...
end

class MyListener::V2; end

Or have a single listener which delegates to a specialized handler:

class MyListener
  def on_event(event)
    case event.fetch(:version)
    when "a"
      V1.call(event) 
    when 2
      V2.call(event)
    else
      raise
  end
end

You could also replace the case with eval, const_get, or constantize (activesupport).

lukeredpath commented 5 years ago

What level of complexity do you anticipate this feature would add? The implementation would seem fairly straightforward I think. I do intend to do a spike on it and see if this is true.

I will explore the idea of some kind of delegating proxy/decorator though.

krisleech commented 5 years ago

What level of complexity do you anticipate this feature would add?

Any additional code has a cost which is why I'm pretty adverse to adding additional features to Wisper.

There are some features I regret adding (e.g. on, with, prefix options) which like this requirement can be handled easily by the listener.

I could see the earlier examples being gemified for easy application to any listener.

Do you have a particular objection to handling this in the listener?