jbogard / MediatR

Simple, unambitious mediator implementation in .NET
Apache License 2.0
11.1k stars 1.18k forks source link

Make Auto Registration of Generic Handlers OPT-IN #1057

Closed zachpainter77 closed 1 month ago

zachpainter77 commented 2 months ago

This is a direct fix for many people's issues regarding the enforcement of constraints for generic request handers having more than two generic type parameters.

Direct fix for: #1055 , #1038

@jbogard I have made this pr so that you can decide what you want to do. Either remove the feature completely or make it Opt-In until we can implement a better resolution strategy.

crnd commented 2 months ago

Is this PR now just removing the protections from the registration process to make errors go away? Won't this now potentially cause users to end up having a lot of unwanted service registrations and potentially timeouts happening during application startup as the feature has been implemented opt-out instead of opt-in?

In my opinion the whole feature should either be removed or then the feature should be made opt-in to make sure that no one ends up getting this enabled by default by just upgrading to a newer minor version. As already stated before, this is anyway too late at this point as 12.3.0 and newer versions have already been released, but the current approach should have been a major version increment.

zachpainter77 commented 2 months ago

Yes it could result in too many registrations. I don't mind making it opt in either.

On Sun, Aug 4, 2024, 6:47 AM Henri Nieminen @.***> wrote:

Is this PR now just removing the protections from the registration process to make errors go away? Won't this now potentially cause users to end up having a lot of unwanted service registrations and potentially timeouts happening during application startup as the feature has been implemented opt-out instead of opt-in?

In my opinion the whole feature should either be removed or then the feature should be made opt-in to make sure that no one ends up getting this enabled by default by just upgrading to a newer minor version. As already stated before, this is anyway too late at this point as 12.3.0 and newer versions have already been released, but the current approach should have been a major version increment.

— Reply to this email directly, view it on GitHub https://github.com/jbogard/MediatR/pull/1057#issuecomment-2267549709, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACL2QUUW5PD6S6CJLOZKI2LZPYWHXAVCNFSM6AAAAABL5OIKNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRXGU2DSNZQHE . You are receiving this because you authored the thread.Message ID: @.***>

zachpainter77 commented 2 months ago

Is this PR now just removing the protections from the registration process to make errors go away? Won't this now potentially cause users to end up having a lot of unwanted service registrations and potentially timeouts happening during application startup as the feature has been implemented opt-out instead of opt-in?

In my opinion the whole feature should either be removed or then the feature should be made opt-in to make sure that no one ends up getting this enabled by default by just upgrading to a newer minor version. As already stated before, this is anyway too late at this point as 12.3.0 and newer versions have already been released, but the current approach should have been a major version increment.

Ok, I have made the feature Opt-In.

image

zachpainter77 commented 2 months ago

@crnd

I have made another PR alongside this one that will completely remove the feature. So now it is only up to @jbogard to decide what PR to merge.

nhwilly commented 2 months ago

This bit me pretty hard when I added MassTransit to my app.

alexkuznetsov commented 1 month ago

@zachpainter77 @jbogard

Hi, any chance of adding this and releasing the nuget package?

After upgrading to version 12.4, we had a few interesting hours to investigate what the ... going on, and after some googling and reading issues on github we found a solution using the RegisterGenericHandlers configuration property.

nhwilly commented 1 month ago

Same here. Installed MassTransit and promptly lost a day tracking it. Backed down a version to fix it.

jbogard commented 1 month ago

Ugh so I think I'll push a version with this behavior opt-in but I want to tackle this much differently. Manually registering and scanning isn't really feasible.