wisper
wisper copied to clipboard
Frozen objects can't publish
I work on a project that uses the values
gem for immutable (hence frozen) value objects in several places. We add behavior to the objects that doesn't mutate their data. Sometimes we generate events of interest that we'd like to publish with Wisper. But due to the lazy-loading of @local_registrations
we can't unless we override initialize
to set @local_registrations = Set.new
and then call super
(which is where #freeze
gets called).
Just this one niggling line of code keeping Wisper from working smoothly for a frozen object.
I'd be happy to put together a pull request—can you tell me what you would or would not want to see in a fix for this issue?
At the moment I can't see a problem with moving @local_registrations, something like:
def initialize(*args)
@local_registrations = Set.new
super
end
def local_registrations
@local_registrations
end
The PR would need a test to make sure publishing works with frozen objects so it doesn't get changed in the future :) Probably in the integration_spec.
That fixes the case where the publisher doesn't have a constructor.
Here are two specs I added to integration_spec.rb
. Adding your suggested code, the first one passes, the second fails because the publisher overwrites #initialize
after the module is included:
it 'publishes events from frozen publishers' do
listener = double('listener')
expect(listener).to receive(:success).with('hello')
command = MyCommand.new
command.freeze
command.subscribe(listener)
command.execute(true)
end
it 'publishes events from frozen publishers with custom constructors' do
listener = double('listener')
expect(listener).to receive(:success).with('constructor argument')
command = MyCommandWithConstructor.new('constructor argument')
command.freeze
command.subscribe(listener)
command.execute
end
So I propose to not support local registrations on frozen objects (silently "just work" at subscribe time; raise an error if someone tries to add one). I think anybody bothering with frozen objects will agree that's reasonable, at least as a first step.