pombreda / google-guice

Automatically exported from code.google.com/p/google-guice
Apache License 2.0
0 stars 1 forks source link

Remove InjectorBuilder #510

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Binder is already an injector builder. In fact, it used to be called 
ContainerBuilder back when Injector was called Container. Configuring the 
Binder (injector builder) through Modules instead of constructing it directly 
enables us to ensure that a Binder is only used once per Injector.

Having two "injector builders" adds unnecessary complexity, especially when we 
already have the Guice class. The methods on InjectorBuilder should move to 
Binder. 

I also liked Jesse's idea of scoping requireExplicitBindings() to the Module. 
Why did we abandon this? We could have a StrictModule support class that calls 
this automatically.

Original issue reported on code.google.com by crazybob...@gmail.com on 1 Jul 2010 at 1:17

GoogleCodeExporter commented 9 years ago
There were a few pros & cons to each approach.  For reference, here are the 
ones brought up when we discussed it a while ago:

1) Add a disableJitBindings() method to Binder.
  Pros: Reuses the Module/Binder framework.
  Cons: Grows the number of "Elements" (and necessary listeners, visitors, etc) more.
        Possibly confusing when using many modules if one has an unexpected disableJitBindings. 

2) Use an InjectorBuilder (issue 395), using a builder pattern.
  Pros: Uses the well-known builder pattern.
  Cons: There'd be two ways of building an Injector (Guice.createInjector, and
Guice.createInjectorBuilder().[...].build())

3) Add a new InjectorModule/Binder framework (for commands specific to how to 
build/control the Injector, such as setting the stage, JIT bindings, etc).
  Pros: Reuses the concept of modules/binders.
  Cons: Would need a whole different hierarchy of classes.

4) Overload Guice.createInjector even more.
  Pros: Simple.
  Cons: Overloading to death is bad.

5) Add a method options() to Binder which exposes disableJitBindings(). Then 
any future options (like disabling circularity proxies) would just be another 
setter on this options. It's a scalable solution.
  (Same pros/cons as #1)

We threw out options 3 & 4 right away, for obvious reasons, and renamed 
"disableJitBindings" to "requireExplicitBindings" at Dhanji's request.

Jesse pointed out that for #1, "The last point is the deal-breaker here. One 
option would be for disallowJitBindings() to apply only to the current module 
and those modules that it installs. As a consequence, I could mix-and-match."

I agree with Jesse on that -- requiring explicit bindings is too strong a thing 
to accidentally have happen when you include a module.  It changes the way your 
entire program behaves, it's not just an additional bit of information.

Unfortunately, I really can't think of any sane way to make 
"requireExplicitBindings" work on a per-module basis, because of the way that 
modules install each other, and that all the extensions (AssistedInject, 
Multibinder) and even built-in extension (@Provides) are installed as modules.  
Requiring explicit bindings on a per-module basis is kind of nonsensical at 
that point.

Jesse did bring up the idea of having a "permit JIT binding" property for each 
binding, but I think there's a few downfalls to the idea, the worst being it 
would increase memory & decrease performance (each binding needs to perform 
additional checks that it doesn't already do).

Original comment by sberlin on 1 Jul 2010 at 3:05

GoogleCodeExporter commented 9 years ago
Did some more thinking on this...

I think the concept of an Injector-wide requireExplicitBindings vs a 
StrictModule-style requireExplicitBindings are, by necessity, two very 
different things.  With something defined on a per-module-basis, it must only 
apply to bindings defined within that module (and perhaps submodules installed 
by that module).  But it has no way of applying to calls of 
Injector.get[Binding|Instance|Provider] -- those would need to be limited to 
something that's defined on "injector-scale".  (It *might* be possible using 
some sort of PrivateModule-like shenanigans... but I'm scared to think about 
that.)

The concept of "StrictModule" could go one of two ways:
    a) require that all bindings in the module are defined in that module (perhaps giving leeway for things defined using requireBinding(...) even if they aren't bound)
 or b) require that all bindings in the module can only inject bindings that were specified in this or other modules

For defining something on a per-module basis, I think option a) is the most 
useful (and also something people have been clamoring for internally), but not 
immediately clear how to do.

Original comment by sberlin on 1 Jul 2010 at 2:46

GoogleCodeExporter commented 9 years ago

Original comment by sberlin on 23 Nov 2010 at 1:29