Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monkey-patching to support 2.x and 3.x #133

Closed
NightOwl888 opened this issue Oct 12, 2015 · 3 comments
Closed

Monkey-patching to support 2.x and 3.x #133

NightOwl888 opened this issue Oct 12, 2015 · 3 comments

Comments

@NightOwl888
Copy link

I have a NuGet package that depends on SimpleInjector.

I am just letting you know that your restrictive use of the ObsoleteAttribute (that is, setting the error argument to true) is causing a problem with supporting your library. As you might already know, the default NuGet behavior will _silently_ upgrade a package to the lowest common version number. So, if another library decides to support only 3.x and that is installed into someone's target project along with mine (which supports 2.x +), NuGet will install 3.x into the project.

This dilemma for those who depend on SimpleInjector has left us with 3 options:

  1. Don't support SimpleInjector 3.x and set the NuGet maximum version < 3.x (which by the way is totally ignored when doing a Package-Update).
  2. Create a separate NuGet package for SimpleInjector 2.x and SimpleInjector 3.x.
  3. Use .NET reflection to work around the ObsoleteAttribute restriction (yuck!)

I went with the 3rd option. But this means that patching the code to support either version has gone from this:

// Extension methods for cross-version support of Simple Injector 2.x and 3.x.

// This will succeed on 2.x and will be bypassed on 3.x
public static void RegisterCollection(this Container container, Type serviceType,
    IEnumerable<Type> serviceTypes)
{
    container.RegisterAll(serviceType, serviceTypes);
}

// This will succeed on 2.x and will be bypassed on 3.x
public static void RegisterCollection<TService>(this Container container, 
    IEnumerable<TService> containerUncontrolledCollection) where TService : class
{
    container.RegisterAll(containerUncontrolledCollection);
}

To this:

// Extension methods for cross-version support of Simple Injector 2.x and 3.x.

// This will succeed on 2.x and will be bypassed on 3.x
public static void RegisterCollection(this Container container, Type serviceType, 
    IEnumerable<Type> serviceTypes)
{
    // container.RegisterAll(serviceType, serviceTypes);
    var method = container.GetType().GetMethod(
        "RegisterAll",
        BindingFlags.Instance | BindingFlags.Public,
        null,
        new Type[] { typeof(Type), typeof(IEnumerable<Type>) },
        null);

    if (method != null)
    {
        method.Invoke(container, new object[] { serviceType, serviceTypes });
    }
}

// This will succeed on 2.x and will be bypassed on 3.x
public static void RegisterCollection<TService>(this Container container, 
    IEnumerable<TService> containerUncontrolledCollection) where TService : class
{
    // container.RegisterAll(containerUncontrolledCollection);
    var method = container.GetType().GetMethods()
        .Where(mi => mi.Name == "RegisterAll")
        .Select(mi => new { M = mi, P = mi.GetParameters(), A = mi.GetGenericArguments() })
        .Where(x => x.A.Length == 1
            && x.P.Length == 1
            && x.P[0].Name == "collection")
        .Select(x => x.M)
        .FirstOrDefault();

    if (method != null)
    {
        var genericMethod = method.MakeGenericMethod(new Type[] { typeof(TService) });
        genericMethod.Invoke(container, new object[] { containerUncontrolledCollection });
    }
}

If you would have just marked the methods obsolete without raising a compile error I wouldn't have to resort to such measures, because I don't have any control over whether the end user will have 2.x or 3.x in their project - that is up to NuGet.

Adding an extension method to mimic the 3.x behavior when 2.x is installed would have been great. Since an instance method takes precedence over an extension method, the fact that 3.x has them would normally make the first code silently ignored. Except when I upgraded to 3.x, the backward-compatibility code failed because the ObsoleteAttribute caused it not to build.

@dotnetjunkie
Copy link
Collaborator

Hi NightOwl,

Thanks for taking the time to report this. The problems you are having is a scenario we discussed during the development of v3. We were aware that with v3 we would break some NuGet packages in very annoying ways (exaggerated by the broken NuGet infrastructure), but after some debate we decided to still go with that route, because this allowed us to make the migration experience much easier for 98% of the users. Although we could have opted for compiler warnings, we decided to go with compiler errors, because we wanted to push developers into the new model and make a completely clean start and keep one consistent registration model for everybody. Again, with the risk of annoying some others, especially library developers.

So, you have all the rights to be annoyed; we deliberately sacrificed you in favor of the majority of the users. This must be very frustrating for you. Still I hope you can understand our position. In order to make progress, we needed to break with the past.

Do note though that in the case of a library that is compiled against Simple Injector v2.x and calls RegisterAll or RegisterSingle, everything will keep working when a consumer of your library moves to v3. In v3, those method overloads simply forward to the corresponding RegisterCollection and RegisterSingleton methods, so your v2 integration package will be upwards compatible. Problem however will start, as you already noticed, when you compile against v3, while trying to keep your package backwards compatible. I think that the use of reflection is in that case your only workaround. And if you happen to depend on methods such as InjectProperties or RegisterAllOpenGeneric, you will even have a much harder time in getting everything working, because support for these methods is dropped completely; they will simply throw an exception.

So other package owners might be in a position that is much worse than you are, although I must admit that you are the first to notify us about this. Nonetheless, I'm glad you posted your workaround here, because likely others will benefit from this.

Thanks for your posting.

@NightOwl888
Copy link
Author

Here is the entire diff for the patch I created for reference in case anyone else finds themselves in the same situation. It doubles the amount of Simple Injector registration code, but I guess if anyone who uses it is so inclined, they could pick either version 2.x or 3.x and get rid of the monkey-patch by editing the code to their liking.

@dotnetjunkie
Copy link
Collaborator

Do note that if your library uses the obsoleted v2 methods, it might break again in the future, because we will eventually remove those methods from v3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants