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

Working with Modules (as namespaces) #2

Open
stergiom opened this issue Sep 27, 2019 · 7 comments
Open

Working with Modules (as namespaces) #2

stergiom opened this issue Sep 27, 2019 · 7 comments

Comments

@stergiom
Copy link

Environment

Crystal 0.31.0 (2019-09-24)
LLVM: 8.0.1
Default target: x86_64-apple-macosx

Issue

Where it works has expected here;

class D1
  def initialize
    puts "Hi from D1"
  end
end

class D2
  def initialize(@d1 : D1); end
end

D2.new(D1.new) #Hi from D1

Syringe.injectable(D1)
Syringe.wrap(D2)

D2.new #Hi from D1

I'm having some trouble getting it to work with Modules;

module A
  class A1
    def initialize
      puts "Hi from A::A1"
    end
  end
end

module B
  class B1
    def initialize(@a1 : A::A1); end
  end
end

B::B1.new(A::A1.new) #Hi from A::A1

Syringe.injectable(A::A1) #ERR: Called macro defined in macro 'injectable'; def Syringe.getA::A1; Error: unexpected token: ::
Syringe.wrap(B::B1)

..and trouble getting it to work from within Modules;

module C
  class C1
    def initialize
      puts "Hi from C::C1"
    end
  end

  class C2
    def initialize(@c1 : C1); end
  end

  Syringe.injectable(C1) #ERR: Called macro defined in macro 'injectable'; return C1.new; Error: undefined constant C1
  Syringe.wrap(C2)
end

C::C2.new(C::C1.new) #Hi from C::C1

C::C2.new

I've had limited success in using Providers here —which works for a single namespace, but not for dependencies across namespaces. Probably something I've missed, is there a canonical way in using this lib with namespaces?

@elliotize
Copy link
Contributor

I found this bug a while back and am working on a patch already I just haven't had time to finish. Let me see if I can get a PR up.

@elliotize
Copy link
Contributor

@stergiom #3 Is my first attempt. The code is pretty gross and will go back but does this fix your issue?

@stergiom
Copy link
Author

Thanks @elliotize, can confirm that this does work for my one-namespace-deep use-case, also works fine for Classes I'd like to have behave as Singletons, currently have that wrapped up for convenience;

macro define_singleton_di_providers(*klasses)
  {% for klass in klasses  %}
    module {{ klass.id.split("::").reject { |k| k == klass.id.split("::").last}.join("::").id }}
      class {{ klass }}Provider
        Syringe.provide({{ klass }})

        @@instance = {{ klass }}.new

        def self.getInstance
          @@instance
        end
      end
    end

    \{% if !{{klass}}.methods.select { |m| m.name == "initialize" }[0].args.empty? %}
      Syringe.wrap({{ klass }})
    \{% end %}
  {% end %}
end

..this too could probably be improved.

@Bonemind
Copy link
Owner

Bonemind commented Oct 3, 2019

Sorry for the radio silence, was on a vacation.
I'll have a look into both the issue, and the PR from elliotize soon

@elliotize
Copy link
Contributor

@stergiom I am working on my Spectrum Shard which is an async/domain event system using Syringe and realized I too have use for a singleton. Are you planning on making a PR to this repo or do you have another Shard you are adding it to.

@stergiom
Copy link
Author

stergiom commented Oct 7, 2019

To be frank, I've only just started fiddling with Crystal and had not considered it. I've dropped a quick'n dirty off at #5, really just convenience macros for using as light IoC containers, but unsure about the scope/direction of Syringe to say whether this would be useful to add;

Syringe.define_singleton_providers(
  UseCase::DoAThing,
  UseCase::DoSomeOtherThing
)

*spec should pass on your module/namespace branch.

If it's just singelton behaviour that you're after, self.getInstance pointing to a class variable that is an instance might be all you need?

@Bonemind
Copy link
Owner

Bonemind commented Oct 7, 2019

For singletons I'd expect a provider which returns an instance to indeed work.
And for the scope of syringe: Primary goal is to provide an easy to digest/use DI shard which does not do much more than that, so it should be complete enough to be usable, but the documentation, and more importantly, setup of the shard itself should be simple and not require any/much config beyond pointing to classes that can be injected and want to be injected into.

Onto the issue itself, good find, I've left a comment on the PR of elliotize, and I'm thinking how we can keep this code a bit more manageable. It looks like the pr of @elliotize works for N deep, so if we can figure out readability i'll merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants