Move Factory, MembersInjector to internal package. about dagger HOT 26 CLOSED

I'd like to confirm that we don't want people injecting MembersInjector
directly. I believe we don't because we can inject the component. But I'd
like anyone who does want such to have a chance to speak up.

On Tue, Nov 11, 2014, 11:16 Jake Wharton [email protected] wrote:

As discussed.

—
Reply to this email directly or view it on GitHub
#63.

from dagger.

I think we found no more than a handful of such examples in google and all
of them could be replaced by injecting the component (if in a provides
method), or just storing the component.

On Sun, Nov 16, 2014, 10:00 Christian Gruber [email protected] wrote:

I'd like to confirm that we don't want people injecting MembersInjector
directly. I believe we don't because we can inject the component. But I'd
like anyone who does want such to have a chance to speak up.

On Tue, Nov 11, 2014, 11:16 Jake Wharton [email protected] wrote:

As discussed.

—
Reply to this email directly or view it on GitHub
#63.

from dagger.

Injecting the component could widen your dependencies, no? If the component
has a getFoo, injectBar and injectBaz method, seems wrong to pass it to
something that just wants to inject bar.

On Sun, Nov 16, 2014, 5:01 AM Christian Edward Gruber <
[email protected]> wrote:

I think we found no more than a handful of such examples in google and all
of them could be replaced by injecting the component (if in a provides
method), or just storing the component.

On Sun, Nov 16, 2014, 10:00 Christian Gruber [email protected] wrote:

I'd like to confirm that we don't want people injecting MembersInjector
directly. I believe we don't because we can inject the component. But
I'd
like anyone who does want such to have a chance to speak up.

On Tue, Nov 11, 2014, 11:16 Jake Wharton [email protected]
wrote:

As discussed.

—
Reply to this email directly or view it on GitHub
#63.

—
Reply to this email directly or view it on GitHub
#63 (comment).

from dagger.

@sameb, I think that's true, but I think the better question is really "who wants to do this in the first place?" All of the usages that I could find for Guice were just inspections using the SPI.

from dagger.

Yup, not very practical or necessary for Guice. Not sure how it'd play out
for Dagger. For the typical object that Android creates and needs members
injection by Dagger, does the user already have the component instance
handy? If so, then yeah, this seems like a moot point.

On Sun, Nov 16, 2014, 9:01 PM Gregory Kick [email protected] wrote:

@sameb https://github.com/sameb, I think that's true, but I think the
better question is really "who wants to do this in the first place?" All of
the usages that I could find for Guice were just inspections using the SPI.

—
Reply to this email directly or view it on GitHub
#63 (comment).

from dagger.

/me misclicked

Typically, those objects instantiated by Android are also tied to lifecycles, so they manage components themselves. So, yes, injection via the component is quite straightforward.

AFAICT, the only reason you'd want a MembersInjector instance would be if you wanted to inject members in an object from another @Injected object. That seems really niche. Plus, if you wanted to support that type of thing, you could always roll your own minimal interface and have a component interface extend it.

from dagger.

Oh, so to be clear, I'm +1 on this proposal.

from dagger.

you could always roll your own minimal interface and have a component interface extend it.

This is great because you can re-implement MembersInjector yourself. The one person who actually needs a general members injector (rather than a typed one) can just create it themselves.

from dagger.

SGTM. (If you're scouring the code to find examples where folks use it in Guice, you may also want to search for people using Injector.injectMembers, since many people don't know that MembersInjector<T> is a thing they can do.)

from dagger.

Bumping this, but we'll chat about it separately.

from dagger.

You know, the thing that I completely overlooked until seeing it now is that † had MembersInjector, so it'd seem a little weird for ‡ to just not have it. Since I think we were a little on the fence, that seems like a reasonable argument in favor of leaving it alone.

Factory can totally move though.

from dagger.

Dagger 1 didn't have components which are a more-explicit members injector.

from dagger.

That's true, but I see it as not really doing any harm and removing it adds cost to migrating from † to ‡.

from dagger.

Agreed. SGTM. I'll close this when I sync from head.

from dagger.

The harm is being the less-valuable of a redundant pair of mechanisms that are in the public API and thus require documentation, explanation, and support.

from dagger.

A semi-crappy thing would be to make dagger.internal.MembersInjector, and make dagger.MembersInjector deprecated and inherit from the former. Then there's a migration path, wtih the deprecation warning to go with it, and a note that it will go away in 2.1.

from dagger.

Never mind - that would require supporting injecting of both interfaces for backward compatibility, and that's ungood.

from dagger.

I'm not clear on how leaving MembersInjector helps migration to Dagger2. IIRC, Dagger2 doesn't support injecting a MembersInjector... so it'd just cause injection errors and confusion, no?

from dagger.

#140 moves Factory into internal.

from dagger.

@sameb, injecting a MembersInjector is something we support and I just sent the CL to test it.

from dagger.

Given that this is a public type that is referenced in user code, I think that's the deciding factor, and we leave MembersInjector as is.

from dagger.

that's the deciding factor

the major version bump disagrees with this

from dagger.

Oh, for sure we could get rid of it. There just didn't seem to be any real benefit. Smaller APIs are nice, but this is already a thing that we already support and moving it to a different package is just feels like churn.

from dagger.

@JakeWharton - to be clear, I didn't mean that the fact that Dagger 1.0 user code references it is the deciding factor, but the fact that it is, currently a supported (and tested) feature of the Dagger 2 code line. If we decided to disable that feature, it would then be in question.

from dagger.

Just stumbled upon this. I only see 16 usages of "MembersInjector<" in google, and most of them seem either like the wrong idea (someone is calling injectMembers(new Foo())) or extremely easy to migrate to just injecting the component or their own type.

I say that because we're working on inlining members injection now, which would mean that dagger's generated factories could remove their dependency on a MembersInjector, really making it obsolete.

If we were to remove it, it could make the internal model much simpler as we could replace all usages of BindingKey with Key and greatly simplify ResolvedBindings.

This would obviously have to go through a deprecation cycle (and I think for something like this we'd probably want something much more than 6 months), but it could be worth it.

from dagger.

from dagger.