This repository was archived by the owner on Jun 1, 2026. It is now read-only.
Repository navigation
Proposal: Better dependency injection behavior - #310
Merged
Merged
Conversation
The service provider should not be created until it is final, larger apps may have a lot of registrations, and it can have a major negative impact on startup performance. Instead, allow the authentication scheme to be specified as a parameter, with a default, and remove from configuration object. Additionally, use the options pattern for FishyFlipOptions instead of registering a singleton object. (Registered as singleton for now, will follow up with conversion to IOptions<T>)
This was done instead of removal, so that anyone using this gets a hint on where to look for the correct way to do this.
By flipping these implementations, we avoid the need to remove the registration and add a new one. Further, by not using TryAddScoped, we allow replacement using the add method since the last registration is what will be used.
Heavy handed for a single property, but allows future additions here too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current code does a few things that aren't good practice:
1: This is an issue in larger apps, where creating the service collection is not the fastest process, and can cause problems when other badly behaved libraries are expecting a single instance, but there are multiple. An example would be older versions of OpenTelemetry, which was not at all a fan of being initialized more than once. Someone could have a valid reason to replace FishyFlipOptions in DI, that involved something that eventually initialized OTEL, which would result in an exception later on (the 2nd call to resolve options).
The authentication schemes are more or less static in the application, and do not need to be configurable outside of code (or when they do need to be, the caller can be responsible for doing that).
2 and 3: The options pattern makes it easier to add tests, it also allows external callers to build on top of the options. A good example here is a secrets provider that is not available during registration. By using the options pattern we allow DI based configuration, instead of manually reading
IConfiguration, or requiring bad behavior to enable the secrets scenario I mentioned.This is still compatible with the current registrations that are not using the authentication scheme properties. It just doesn't force configuration to be done via lambda/action, and allows future configuration by the caller using .AddOptions/Configure/etc.
These relatively minimal changes will make FishyFlip play much nicer with ASP.NET, and
IServiceCollection/IServiceProviderNOTE:
The obsolete properties can just be removed, they're not being used anywhere now. I left them to allow people who are upgrading to get a hint on where to go if they are customizing this (I'm sure this is actually quite rare).