Change module scope so that one per request is created

[gkinsman]

Hi there! I'm not sure if it was an intentional change to remove the module-resolution-per-request behaviour, but here we are!

It was changed in this commit as part of some optimisations. In fact, pulling out the resolution from the closure meant that the startup-scoped module instance is used for every request. An optimisation indeed, but removes the ability to resolve per-request dependencies from the constructor.

I've reverted that behaviour and added a test to ensure this behaviour is tracked in the future.

[jchannon]

I was scratching my head about this and to write some more tests. I removed your change and added more tests and it shows module injected dependencies still work as expected even though the module isn't resolved per request.

[gkinsman]

Thanks for your time @jchannon! It looks like the dependencies in the tests you added always return new guids when asked - is that what you intended? My expectation was that types passed in from the constructor would be new instances for each request.

I've updated your added tests to give the transient/scoped dependencies instance ids per instance and the tests check to make sure they're difference isntances, which is what I'd expect; they now fail.

Really appreciate you looking at this!

[jchannon]

Just pushed a commit and put your change back in. Without it transient and scoped lifetimes were not behaving as expected ie. they were singleton. Added more tests but makes me wonder whether the whole scope creation in CarterExtensions needs some love now. It was put in to fix a singleton problem but it's a bit confusing I think :)

Just got to try and make the other tests pass now as they are trying to resolve dependencies in modules that aren't part of the tests and therefore don't have the DI registrations

[gkinsman]

Sounds pretty good! I'd be happy to have a crack at it tomorrow evening. I think you're right, the difference between the startup scope and per-request scope isn't super obvious - will make that the goal for the refactor.

[jchannon]

Here's the original reason it was done

https://github.com/CarterCommunity/Carter/pull/62

On Mon, 25 Feb 2019 at 22:05, George Kinsman notifications@github.com wrote:

Sounds pretty good! I'd be happy to have a crack at it tomorrow evening. I think you're right, the difference between the startup scope and per-request scope isn't super obvious - will make that the goal for the refactor.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CarterCommunity/Carter/pull/172#issuecomment-467202767, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGapv8dkTJGb0dTARhzhhFf4wOxcNFwks5vRF4wgaJpZM4bN6se .

[gkinsman]

So @jchannon I've had a crack at a new configuration API. Basically, the default of 'find and register everything in this assembly' is still there, and still the default behaviour. There's now an overload of AddCarter that accepts an Action<CarterConfigurator> with the following methods:

CarterConfigurator WithModule<TModule>() where TModule : CarterModule CarterConfigurator WithModules(params Type[] modules) CarterConfigurator WithValidator<T>() where T : IValidator CarterConfigurator WithValidator(params Type[] validators) CarterConfigurator WithStatusCodeHandler<T>() where T : IStatusCodeHandler CarterConfigurator WithStatusCodeHandlers(params Type[] statusCodeHandlers) CarterConfigurator WithResponseNegotiator<T>() where T : IResponseNegotiator CarterConfigurator WithResponseNegotiators(params Type[] responseNegotiators)

Adding each type of thing has an overload for an individual type, or a collection, so can be fed from reflecting over assemblies. The default behaviour feeds into this API via its existing signature and it all gets fed into a new AddCarter:

static void AddCarter(this IServiceCollection services, IEnumerable<Type> moduleTypes, IEnumerable<Type> validatorTypes, IEnumerable<Type> statusCodeHandlerTypes, IEnumerable<Type> responseNegotiators)

[gkinsman]

Nice one! I suspect the most common use case will still be assembly scanning, but at least now you can be specific about what's going in there.

I wonder if there's any other flexibility we can add here, for example it might be nice to be able to use a DI container's registrations instead of binding them here - almost like declaring that a module has already been registered with the container, so we don't need to re-add it.

Thoughts?

[jchannon]

I'm not sure I can see a use case of registering Carter related types to the container before calling AddCarter. I think any types should be specified with the configurator class you wrote

[gkinsman]

I think you're right - MVC works this way too so it'd be weird to deviate from the expectation they've set.