rubiconmd / injectable

Opinionated and declarative Dependency Injection library for ruby.
https://rubygems.org/gems/injectable
MIT License
34 stars 5 forks source link

Instance variables are kept across SO invocation #21

Open thebigw4lrus opened 4 years ago

thebigw4lrus commented 4 years ago

Context

Declarations of instance variables within a SO.

Problem

Instances variables are kept across SO invocation. It is only reproduce-able when an inline dependency is declared like:

dependency :something, class: SomeClass

How to Reproduce it

class Example::Resolver
  include Injectable

  argument :value

  def call
    result
  end

  private

  def result
    @variable ||= value
  end
end

class Example::Caller
  include Injectable

  dependency :resolver, class: Example::Resolver

  def call
    puts "Result 1 -> #{resolver.call(value: 1)}"
    puts "Result 2 -> #{resolver.call(value: 2)}"
  end
end

> Example::Caller.call
Result 1 -> 1
Result 2 -> 1
=> nil

Note: When the dependency is declared like dependency(:resolver) { Example::Resolver } this does not happen, and all instances variables are cleared up after across execution.

thebigw4lrus commented 4 years ago

I found this long time ago. From the dependency inversion point of view, it was kind of okayish to me to keep the instance variables as we pass an 'instance' to the caller constructor(in which case we should take that into account when we design SO). But now, thinking this twice and checking that the behaviour of dependency(:resolver) { Example::Resolver } (with block, complex setup) differs, I think worth having a look and discussing it.

thebigw4lrus commented 4 years ago

Oh, I just noticed that I was miswriting the 'block' dependency. The Readme states that we should instantiate the object from within the block..

So I can confirm that doing this will reproduce the issue too:

dependency(:resolver) { Example::Resolver.new }

I don't know if we can call it issue, as my main concern was the behaviour not being consistent across all possible way to write the dependency. If this is not an issue, we need to be careful when we write our SO's.

Papipo commented 4 years ago

Hi @thebigw4lrus! I am actually aware of this since I wrote the lib. I already thought about a couple of ways of solving it:

Not sure which approach is better, tbh. For the time being, it's recommended to be careful when writing SOs, but you would have to be careful when writing any code that memoizes and also receives arguments, no?

thebigw4lrus commented 4 years ago

Yup, deffo!, I would also add: when the memoized value(single value) depend on those arguments.

Also I think memoization of single values takes a bigger transcendence when it comes to SOs. It is like: If I have a service that does something, I wouldn't expect it to memoize things that might change with its arguments (unless there is some special circumstances of course, like keeping a connection to somewhere else, or something like that).

# I wouldn't expect to memoize salt_added_by_chef value
# as  carbonara involve bacon, which might have more salt
# than the clams (vongole)
PastaCooker.call(sauce: 'carbonara')
PastaCooker.call(sauce: 'vongole')
# Note: salt_added_by_chef, is calculated at execution time.
Papipo commented 4 years ago

I need to invoke @jantequera and @iovis here to know their opinion.