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

Add support for singletons with auto-register and auto-inject #21

Closed
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@rx
Copy link
Contributor

rx commented Jul 25, 2016

Hey Guys,

I have a 'repository' that is is a singleton and I ran into an issue trying to use a singleton using auto_registration and auto-inject.

This change allows a class like the following you to be used with the dry-component auto-register and auto-inject:

class OneOfMe
  include Singleton
end

The change does not change the behavior for any other classes besides singleton's. (At least that was my intention.)

There are two changes - one is to the default loader to see if the object responds to :instance and not :new. This means it quacks like a singleton and we call .instance(_args) instead of .new(_args).

The other change is to the load_component method, where it was calling .new directly instead of using the loader instance method.

There is another location that probably should be changed in the auto_registration! method where a block is passed. It loads the class before calling the block. This means that if you pass a block in you can't lazy load your components. I didn't commit this change because it changes the default behavior and I didn't need it to get singleton behavior working, but I think it should be passing the loader to the block and letting the block decide how to load/register.
Like so:

diff --git a/lib/dry/component/container.rb b/lib/dry/component/container.rb
index a2aae2b..6f25eb6 100644
--- a/lib/dry/component/container.rb
+++ b/lib/dry/component/container.rb
@@ -78,7 +78,7 @@ module Dry
             Kernel.require component.path

             if block_given?
-              register(component.identifier, yield(component.constant))
+              register(component.identifier, yield(component))
             else
               register(component.identifier) { component.instance }
             end

I added specs for the changes.

I also added RubyMine .idea to the .gitignore.

The changes are based off the v0.3.0 release, but should merge into mainline with little trouble. (I ran into an Inject issue when I pulled from mainline.)

Thanks for considering this change. I'm really enjoying creating clean architecture/code using the dry ecosystem, thanks for all your hard work.

@AMHOL

This comment has been minimized.

Copy link
Member

AMHOL commented Jul 26, 2016

You can already do this with a custom loader, the idea being that the simple default covers the 95% and the other cases can configure it. See https://github.com/dry-rb/dry-component/blob/39f39f8e76eefff860d7959c385886e059421e5a/spec/unit/container/auto_register_spec.rb#L40-L64

@timriley

This comment has been minimized.

Copy link
Member

timriley commented Jul 26, 2016

So I guess the question here is whether this makes sense to go in our default loader. Is it something we want to offer out of the box?

I also saw @solnic in gitter mentioning that quite a many objects could be registered as singletons with the container, even if they're not Singletons in the strict sense (i.e. not responding to .instance). I wonder if there's some way this could be considered alongside my question above?

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Jul 26, 2016

I was against adding support for handling singleton instantiation, but being singleton-aware is OK and we should probably support this as a built-in, optional feature.

@timriley timriley force-pushed the dry-rb:master branch from 69a30e4 to 05897a3 Jul 26, 2016

timriley added some commits Jul 26, 2016

Require at least Ruby 2.1
This is dry-auto_inject’s minimum now
@fran-worley

This comment has been minimized.

Copy link

fran-worley commented on 895d245 Jul 26, 2016

Shouldn't that read:
Compare v0.4.0...v0.4.1

?

This comment has been minimized.

Copy link
Member

timriley replied Jul 26, 2016

Thanks @fran-worley, fixed :)

timriley added some commits Jul 26, 2016

Merge pull request #22 from dry-rb/support-default-namespace-when-res…
…olving-individual-components

Support default namespace when loading files using shorthand identifiers
@rx

This comment has been minimized.

Copy link
Contributor

rx commented Jul 26, 2016

@AMHOL Yes, I started with a custom loader. The second half of this is is to change auto-inject to use the loader:

@@ -133,7 +133,7 @@ def self.load_component(key)

            import_container(src_key, src_container)
          else
 -          require_component(component) { |klass| register(key) { klass.new } }
 +          require_component(component) { |klass| register(key) { component.instance } }
          end
        end

The above change is enough to support singletons with custom loaders.

@solnic When you say build-in, optional feature do you mean it should be configurable?

@timriley before using dry-component I was using a custom container registry and resolver to get singleton behavior. I believe this is a common use case.

timriley added some commits Jul 26, 2016

subject(:component) { Dry::Component::Loader.new('test/bar') }

it_behaves_like 'a valid component'
end

This comment has been minimized.

@solnic

solnic Jul 31, 2016

Member

Something's wrong with indentation here

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Jul 31, 2016

I just realized that having it configurable would be difficult to use, since loaders are per-component IIRC, so I think it's OK to make default loader singleton-aware. FWIW we want to use singletons in most of the cases anyway :)

@timriley you ok with merging this in?

@timriley

This comment has been minimized.

Copy link
Member

timriley commented Jul 31, 2016

@solnic Yep, I'm good with merging this. (I can take care of this on Monday if you'd like!)

In general, I think it'd be good to make it easier for more objects to be handled as singletons, so this is a good first step :)

@timriley

This comment has been minimized.

Copy link
Member

timriley commented Jul 31, 2016

@rx You happy take a final pass at this and get those merge conflicts sorted out? Thanks!

@timriley timriley changed the title Added support for singleton's with auto-register and auto-inject Add support for singletons with auto-register and auto-inject Aug 1, 2016

timriley added some commits Aug 1, 2016

Merge pull request #24 from dry-rb/skip-load-component-for-pre-existi…
…ng-keys

Return immediately from load_component if the key already exists
@rx

This comment has been minimized.

Copy link
Contributor

rx commented Aug 1, 2016

Yes, I'm happy to get the merge conflicts sorted out.

@solnic https://github.com/solnic

There is another location that probably should be changed in the
auto_registration! method where a block is passed. It loads the class
before calling the block. This means that if you pass a block in you can't
lazy load your components. I tried to get the non-block behavior by passing
a block and I couldn't get the same base behavior because it does not allow
lazy loading.

I didn't commit this change because it changes the default behavior and I
didn't need it to get singleton behavior working, but I think it should be
passing the loader to the block and letting the block decide how to
load/register.

I believe it should be changed, but it changes what is passed to the block,
so for anyone using block auto_registration! it will change the behavior. I
have no idea if this is widely, or scarcely used. IMO it should be changed.
I don't think the current implementation is very useful if you can't
reproduce the default behavior with it.

Here is the additional proposed change:

diff --git a/lib/dry/component/container.rb b/lib/dry/component/container.rb
index a2aae2b..6f25eb6 100644
--- a/lib/dry/component/container.rb
+++ b/lib/dry/component/container.rb
@@ -78,7 +78,7 @@ module Dry
Kernel.require component.path

         if block_given?
  •          register(component.identifier, yield(component.constant))
    
  •          register(component.identifier, yield(component))
         else
           register(component.identifier) { component.instance }
         end
    

On Sun, Jul 31, 2016 at 1:41 AM, Tim Riley notifications@github.com wrote:

@rx https://github.com/rx You happy take a final pass at this and get
those merge conflicts sorted out? Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#21 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEoU_FwLIsmXyciunVZIlAl2EdfdAITks5qbDWPgaJpZM4JUaac
.

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Aug 1, 2016

I believe it should be changed, but it changes what is passed to the block,
so for anyone using block auto_registration! it will change the behavior.

Which "block auto_registration!"?

@rx

This comment has been minimized.

Copy link
Contributor

rx commented Aug 1, 2016

When I was spelunking the code to put in support for singletons. I tried using auto_registration! and passing a block. The current implementation optionally takes a block and yields to that block in the container registration call. At the time I thought I might be able to control the lifecycle of a singleton by providing code that is put in the container registry. Passing a block to the method would have been useful, except the parameter that is passed to that block is a call to loader.constant. That, in essence, loads every class at the time of registration. (The default behavior lazy loads classes.) I had some files in my loaded tree that did not strictly follow the module=directory and class=filename naming convention and the container failed to 'boot'. As a result, I was unable to pass a block and get the same lazy loading behavior. I'm recommending that a single line of code be changed that registers the object to pass the loader instead. If anyone is even using that construct, it is a more useful implementation. It really is no big deal, but since I had noticed the difference in behavior from the default behavior, I thought I'd make the recommendation. You could equally argue that the loader provides the necessary flexibility and the code that calls the block is unnecessary.

Here is the full implementation of the code I'm referring to:

 def self.auto_register!(dir, &_block)
        dir_root = root.join(dir.to_s.split('/')[0])

        Dir["#{root}/#{dir}/**/*.rb"].each do |path|
          component_path = path.to_s.gsub("#{dir_root}/", '').gsub('.rb', '')
          config.loader.new(component_path).tap do |component|
            next if key?(component.identifier)

            Kernel.require component.path

            if block_given?
              register(component.identifier, yield(component.constant))
            else
              register(component.identifier) { component.instance }
            end
          end
        end

        self
      end

And here is the single line change I'm proposing:

--- a/lib/dry/component/container.rb
+++ b/lib/dry/component/container.rb
@@ -78,7 +78,7 @@ module Dry
             Kernel.require component.path

             if block_given?
-              register(component.identifier, yield(component.constant))
+              register(component.identifier, yield(component))
             else
               register(component.identifier) { component.instance }
             end
@solnic

This comment has been minimized.

Copy link
Member

solnic commented Aug 1, 2016

This sounds reasonable! @timriley would that work for you? It shouldn't
cause any issues with existing apps.

On 1 August 2016 at 22:05:11, Russell Edens (notifications@github.com)
wrote:

When I was spelunking the code to put in support for singletons. I tried
using auto_registration! and passing a block. The current implementation
optionally takes a block and yields to that block in the container
registration call. At the time I thought I might be able to control the
lifecycle of a singleton by providing code that is put in the container
registry. Passing a block to the method would have been useful, except the
parameter that is passed to that block is a call to loader.constant. That,
in essence, loads every class at the time of registration. (The default
behavior lazy loads classes.) I had some files in my loaded tree that did
not strictly follow the module=directory and class=filename naming
convention and the container failed to 'boot'. As a result, I was unable to
pass a block and get the same lazy loading behavior. I'm recommending that
a single line of code be changed that registers the object to pass the
loader instead. If anyone is even using that construct, it is a more useful
implementation. It really is no big deal, but since I had noticed the
difference in behavior from the default behavior, I thought I'd make the
recommendation. You could equally argue that the loader provides the
necessary flexibility and the code that calls the block is unnecessary.

Here is the full implementation of the code I'm referring to:

def self.auto_register!(dir, &_block)
dir_root = root.join(dir.to_s.split('/')[0])

    Dir["#{root}/#{dir}/**/*.rb"].each do |path|
      component_path = path.to_s.gsub("#{dir_root}/", '').gsub('.rb', '')
      config.loader.new(component_path).tap do |component|
        next if key?(component.identifier)

        Kernel.require component.path

        if block_given?
          register(component.identifier, yield(component.constant))
        else
          register(component.identifier) { component.instance }
        end
      end
    end

    self
  end

And here is the single line change I'm proposing:

--- a/lib/dry/component/container.rb
+++ b/lib/dry/component/container.rb
@@ -78,7 +78,7 @@ module Dry
Kernel.require component.path

         if block_given?
  •          register(component.identifier, yield(component.constant))
    
  •          register(component.identifier, yield(component))
         else
           register(component.identifier) { component.instance }
         end
    


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#21 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAEKgLmk-zgtKdAkuK266293GXdyZBnks5qblF3gaJpZM4JUaac
.

@timriley

This comment has been minimized.

Copy link
Member

timriley commented Aug 1, 2016

This change sounds smart indeed! @rx, would love for you to put it forward, either in this PR or another, I don't mind :)

rx added some commits Aug 2, 2016

@rx

This comment has been minimized.

Copy link
Contributor

rx commented Aug 2, 2016

@timriley I made a pass at pulling this forward. I'm not sure I did it correctly. I merged upstream/master into my forked master. Then I merged those changes to this branch (singleton_fix) and fixed conflicts. The commit list of this pull request now shows those items merged from origin/mainline. I haven't done this enough to know if this is expected or not. Let me know if I need to open a new PR or not. Thanks.

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Aug 2, 2016

@rx this doesn't look right, we shouldn't have commits from master in this branch. The simplest way is to do git rebase -i master and squash your commits into one, then just force-push to this branch. Before you do that you need to clean up your branch so it doesn't include commits from master, otherwise it's gonna get messy :) Lemme know if you need help with that.

@rx

This comment has been minimized.

Copy link
Contributor

rx commented Aug 3, 2016

@solnic Roger that. Didn't seem right. I should have a cleaned up PR
tomorrow.

On Tue, Aug 2, 2016 at 1:29 PM, Piotr Solnica notifications@github.com
wrote:

@rx https://github.com/rx this doesn't look right, we shouldn't have
commits from master in this branch. The simplest way is to do git rebase
-i master and squash your commits into one, then just force-push to this
branch. Before you do that you need to clean up your branch so it doesn't
include commits from master, otherwise it's gonna get messy :) Lemme know
if you need help with that.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#21 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEoU5URfFqUpmbmeGcsbQ0r5jVSEiwVks5qb35mgaJpZM4JUaac
.

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Aug 4, 2016

Closed in favor of #25

solnic added a commit that referenced this pull request Aug 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment