search

ef-labs / vertx-guice

Create Vert.x Modules and Verticles with dependency injection using Guice
MIT License
59 stars 24 forks source link

Request: Add an explicit binding for the Verticle class being injected to GuiceVerticleLoader #16

Closed dano closed 6 years ago

dano commented 6 years ago

This probably seems like an odd request, but bear with me:

I have a use-case where I need to inject the Injector into some of my Verticles. Now, vertx-guice creates a child injector for every Verticle it injects, but because of https://github.com/google/guice/issues/973, if I inject the Injector into one of those, I end up injecting the parent Injector, instead. This breaks things if I pass custom modules using "guice_binder", because those modules won't be present in the injected Injector.

The recommended work-around for this issue is to add an explicit binding to the class that's going to inject the Injector into the child Injector. Right now, for me to do that, I have to add a bind(MyInjectorUsingVerticle.class); binding to a Module and pass that to the "guice_binder" key for every instance of a Verticle that needs to inject the Injector. Even ones that don't want to use any custom bindings. Failing to add it to all of them will prevent Verticles that do want custom bindings from working.

In my case, I have a lot of instances of a particular Verticle that does this, and it's a pain to have to remember to add this custom binder to all of them. By adding the bootstraps.add(binder -> binder.bind(clazz)) to vertx-guice itself, this isn't necessary. I can't think of any adverse side effects - the only tests that failed when I made this change were ones that counted the number of modules loaded, and I added a new module, which is expected. All it does is ensure that the binding for the injected Verticle class gets created in the child Injector, rather than the parent. This probably makes no difference in 99% of use-cases, but it makes life, much, much easier in mine.

Does this seem reasonable to you? If so, I can provide a PR. If not, no big deal, I'll just use a forked version that adds this change in my project. But this was a very confusing gotcha for me to debug, and I figure it might be useful for other people if it's available upstream.

dano commented 6 years ago

The more I thought about this, this more I thought it's probably not a good idea. Closing it...