Return method object instead of monkey patching

[rewritten]

When an alternative method (not :call) is passed, instead of adding a new method we can return a bound method, which will already respond to :call as expected.

This should solve #16 and probably there is no need to raise Injectable::MethodAlreadyExistsException anymore

[iovis]

Hi @rewritten!

Welcome to the Injectable gem, thanks a lot for your contribution!

I'll try to review this ASAP. In the meantime, do you mind adding a regression test?

[codeclimate[bot]]

Code Climate has analyzed commit 9b59b3fd and detected 0 issues on this pull request.

View more on Code Climate.

[rewritten]

Done, I was waiting for having a development environment at hand (previous commit was done from a mobile device, I couldn't run specs)

[rewritten]

The additional spec was red in master, FWIW

[rewritten]

Now the Injectable::MethodAlreadyExistsException is not needed anymore but someone might rely on it, so I'd rather not remove the exception, even if it means that objects with a call method won't be allowed to be used like

dependency(:some_const, call: :arity) { ->(x) { "some proc" } }

[iovis]

Can confirm the regression test works!

[iovis]

On a previous version of the gem we returned a lambda that would delegate the call to the method but we changed it because it felt a bit confusing not to have an instance of the actual dependency when debugging; but, to be honest, if you need the actual instance you probably wouldn't use the :call functionality here and just handle the raw dependency directly, so this is probably fine.

I think I agree with Injectable::MethodAlreadyExistsException not being necessary anymore as we're not modifying the actual object anymore outside of their context (we had a problem with ActionCable and a Notify::Websocket service we had that was modifying a singleton that implements the interface for Rack when we made this change). I don't think removing it can break anyone's code as it literally prevents you from using the :call functionality if there's a conflict.

We should still think about whether there's other ways someone might be leveraging that exception just in case but I think you're correct in your assumption.

[rewritten]

it felt a bit confusing not to have an instance of the actual dependency when debugging

With the public_method implementation you can retrieve the original object with dep.receiver. But we can also return a real proxy using SimpleDelegator, which will absolutely quack like the original object except for the additional :call method:

C = Class.new(SimpleDelegator) do
  define_method(:call) do |*args, **kw|
    __getobj__.public_send(call_method, *args, **kw)
  end
end

C.new(Object).name
# => "Object"
C.new(Object).call
# => "Object"

This has still the same problems as the wrapper proc:

• arity of the original method is not retained
• the object is not of the expected class

but at least one can use all of the original object's methods.

Comparison of implementations

Feature	Wrapper lambda (previous)	:define_singleton_method (current)	:public_method (this PR)	SimpleDelegator
arity	❌	❌	✅	❌
original object	❌	✅	✅ dep.receiver	✅ dep.__getobj__, dep.any_method_except_for_call
debug-friendly	❌	✅	🌤	✅ dep.to_s and dep.inspect are delegated too
is_a?(original class)	❌	✅	❌	❌
leaves constants untouched	✅	❌	✅	✅
allows to override existing :call method	✅	❌	✅	✅

If you feel that the proxy solution is more ergonomic than the method object, I can definitely change the implementation - making sure only one SimpleDelegator class is created for each dependency declaration.

[iovis]

I think your public_method proposal makes more sense, tbh.

If someone needs access to the instance state or to more than one way to interface with it they would just declare the dependency in the default way:

dependency :my_dependency

def call
  my_dependency.state
  my_dependency.other_method
end

When a developer chooses to alias :call is because they explicitly want to interface with that object in one way and one only and they want to invert the dependency to comply with the common interface, which means they don't intend to use it in any other way.

dependency :my_dependency, call: :other_method

def call
  my_dependency.call
end

It is surprising when debugging, but maybe that's the wrong way of thinking about it. If someone needs to debug the instance at a deeper level they can either remove the :call temporarily or use the my_dependency.receiver in their debugging environment.

I also like the simplicity of implementation of :public_method and the fact that it deprecates the need for raising the Exception as we're being more subtle about the behavior change.

[rewritten]

Got it and I agree.

Just in case, this will also be quite simple (I have just reasoned about it, to make the proxy solution a bit more manageable)

class Injectable::Proxy < SimpleDelegator
  def initialize(delegate, proxied_method)
    @proxied_method = proxied_method
    super(delegate)
  end

  def call(*args, **kw)
    __getobj__.public_send(@proxied_method, *args, **kwargs)
  end
end

# in Dependency
def wrap_call(the_instance)
  # ...

  # instead of `the_instance.public_method(call)`
  Injectable::Proxy.new(the_instance, call)
end

This will not require a different proxy class for each dependency, because the method to be proxied is stored in the proxy's state.

[iovis]

I ran this version of injectable against our platform and the tests passed, so this is ready to go on my end

[iovis]

I think the public_method approach is the right way for now and we can see how it evolves and how it feels knowing the simple delegator might be an option to consider if it falls short, but I think this approach is sound for now.

I'll create an issue to discuss whether Injectable::MethodAlreadyExistsException should be removed in a future release.

Thanks a lot for your contribution @rewritten!