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

Instance variables are kept across SO invocation #21

Open
thebigw4lrus opened this issue Sep 14, 2020 · 5 comments
Open

Instance variables are kept across SO invocation #21

thebigw4lrus opened this issue Sep 14, 2020 · 5 comments
Labels
bug Something isn't working invalid This doesn't seem right question Further information is requested

Comments

@thebigw4lrus
Copy link

thebigw4lrus commented Sep 14, 2020

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 thebigw4lrus added the bug Something isn't working label Sep 14, 2020
@thebigw4lrus
Copy link
Author

thebigw4lrus commented Sep 14, 2020

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
Copy link
Author

thebigw4lrus commented Sep 14, 2020

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.

@thebigw4lrus thebigw4lrus added question Further information is requested invalid This doesn't seem right labels Sep 14, 2020
@Papipo
Copy link
Contributor

Papipo commented Sep 30, 2020

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

  • Disallowing further calls of SOs.
  • Provide a blank slate each time call is called.

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
Copy link
Author

thebigw4lrus commented Sep 30, 2020

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
Copy link
Contributor

Papipo commented Oct 1, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants