instantiateUnmapped dependency tree use cases

[Stray]

Hi Till,

as I see it there are 2 usecases:

1) "Instantiate a Thing, using the injector mappings for the dependency tree"

2) "Instantiate a Thing, using fresh instances for the dependency tree"

Any hybrid of these would need to be implemented by the user using a custom dependency provider.

So, the method is now:

instantiateUnmapped(type : Class, useMappingsForDependencyTree : Boolean = true)

with uses:

1) instantiateUnmapped(Thing);

2) instantiateUnmapped(Thing, false);

In case (1), if you have unmapped dependencies in the tree, you need to provide a suitable fallbackProvider and it will choke if this can't provide for everything you need.

In case (2), the injector will assume that you want to use the FreshInstanceProvider for all dependencies, even if they have been mapped previously.

It was fairly straightforward to get the tests passing. I've created the FreshInstanceProvider and in case (2) it simply generates a new injector that has no mappings and then passes off the work to that. You might want to cache that injector - I've just done the simplest thing that can work.

The FreshInstanceProvider is saving the type passed to the prepareNextRequest(...) method and using injector.instantiateUnmapped(...) to fulfil this. If prepareNextRequest and apply were ever non-sequential this could be a problem - but I can't see how they could be.

Ta!

Stray

[tschneidereit]

I'm not sure I understand use case 2. Why wouldn't you just create a second injector with the FreshInstanceProvider set as its default provider? Also, I'm fairly certain that that wasn't the remaining task we talked about at try{harder}.

[Stray]

"Why wouldn't you just create a second injector with the FreshInstanceProvider set as its default provider?"

It's just a wrapper for that workflow really. My main reason for settling for doing both via the same method is that it draws attention to the ambiguity at the point of use. IMO, the parameter on the instantiateUnmapped method, all be it optional, makes it clearer that when you instantiateUnmapped you will still, by default, use mappings for the dependency tree.

Normally I don't like optional params, but I think in this case it beats pretending that the method name isn't still slightly ambiguous. I did play around with other API possibilities (including just leaving it manual) but they all obscured the ambiguity rather than exposing it.

In addition, if we leave it to the developer to create their own empty injector, they'd then potentially be injecting 2 injectors into their commands, assuming they don't want the overhead of creating an injector mid-command. And those injectors would then need to be named. Ew. (I haven't cached the empty injector used in the Injector but my expectation was that it should be kept and reused rather than thrown away - possibly even as a static property so that child injectors etc all use the same instance?)

I am pretty sure that part (1) was what we left todo at try{harder} - in that the ability to deal with the dependency tree was the part that wasn't working, and I can't find anything else that isn't working... (famous last words).

[tschneidereit]

After a long discussion about this with @darscan, I have decided not to take this pull request. While the functionality certainly is interesting, I think that, in the end, it is better served by manually providing two separate injectors, one of which only contains a default fallback provider instantiating new instances for all mappings.

The reason is that the use-case is sufficiently rare and easy to manually support to make it undesirable to bloat up Swiftsuspenders' code-base to automatically support it.

I feel bad about not taking such a big, well thought-out and thoroughly tested change, but I believe that it is for the best of the code-base.

[Stray]

Thanks Till, understood on the "it's outside the 80/20 rule" domain for sure.

Don't feel bad! The purpose of fully thinking out and testing the change was always to better enable the decision 'is this worth it?' Over the previous four years (!) or so it had been mooted and discussed loosely but never fully scoped - in the scoping we achieved certainty that it was too involved to be reasonable within the standard implementation. At the very minimum we now have a commit to point people at to say "this is why it doesn't work this way... ".

However, I think cherry-picking the more specific InjectorErrors (on sealing etc) would be justified, as these definitely add functionality in terms of users being able to apply multiple rulesets depending on which features of their app are activated, and I don't think they 'bloat' anything.

I would also suggest cherry picking the test that I added that confirmed things that were working but previously not covered by tests (to do with keys for injector sealing being unique).

I'm not sure what the status of FreshInstanceProvider is in your proposed manual solution, as that seems to be required for the second-injector approach? Or am I missing something there?

It might be that you've actually cherry picked all those things and I've somehow missed that.

Anyway - thanks for your help in the process of investigating it - I learned a great deal from having to work with your code in detail.

[tschneidereit]

However, I think cherry-picking the more specific InjectorErrors (on sealing etc) would be justified, as these definitely add functionality in terms of users being able to apply multiple rulesets depending on which features of their app are activated, and I don't think they 'bloat' anything.

I would also suggest cherry picking the test that I added that confirmed things that were working but previously not covered by tests (to do with keys for injector sealing being unique).

Oh right: I totally forgot to cherry-pick those. Thanks for reminding me: I'll definitely do that.

I'm not sure what the status of FreshInstanceProvider is in your proposed manual solution, as that seems to be required for the second-injector approach? Or am I missing something there?

Right, I'll also add that to the selection of bundled providers, thanks.

Anyway - thanks for your help in the process of investigating it - I learned a great deal from having to work with your code in detail.

Thank you again for doing all the work on this!

[Stray]

Also an excellent lesson that sometimes the only way to know whether you actually want/need a feature is to build it and see if it sticks. (How very post-modern).