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

Make #unsubscribe functional when both block-based subscribers and listener objects are used #17

Merged

Conversation

Aerdayne
Copy link
Contributor

@Aerdayne Aerdayne commented Apr 26, 2024

Reproduction script:

#!/usr/bin/env ruby

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'dry-events', require: 'dry/events', path: '.'
end

class Monitor
  include Dry::Events::Publisher["Monitor"]
end

monitor = Monitor.new

AGGREGATOR = []

class Listener
  def on_test(event)
    p "listener: #{event}"
    AGGREGATOR << event
  end
end

listener = Listener.new
monitor.register_event(:test)

monitor.subscribe(:test) do |event|
  p "block #{event}"
  AGGREGATOR << event
end
monitor.subscribe(listener)

monitor.publish(:test, foo: "bar")

begin
  monitor.unsubscribe(listener)
rescue => e
  p e
  # => #<NoMethodError: undefined method `receiver' for #<Proc:0x000000010389a2e8 bin/console:29>>
end

Since Proc instances do not have a receiver, in case the same bus instance is subscribed to with a block and an event listener object, #unsubscribe raises an exception unless the target listener object exists and precedes the block-based subscribers in the #listeners hash.

@Aerdayne
Copy link
Contributor Author

Hi, is there any chance this gets merged in foreseeable future?

@timriley
Copy link
Member

Hi @Aerdayne, yes, I'll try and get this looked at in the next little while.

@flash-gordon flash-gordon merged commit 8d80ea6 into dry-rb:main Jan 4, 2025
6 checks passed
@flash-gordon
Copy link
Member

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants