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

Proposal: Inject existing object into MEF2 #29400

Closed
gthvidsten opened this issue Apr 27, 2019 · 13 comments · Fixed by #66364
Closed

Proposal: Inject existing object into MEF2 #29400

gthvidsten opened this issue Apr 27, 2019 · 13 comments · Fixed by #66364
Labels
api-approved API was approved in API review, it can be implemented area-System.Composition enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@gthvidsten
Copy link

MEF1 has method ComposeExportedValue<T>(T exportedValue). MEF2 should have a similar functionality in System.Composition.Hosting.ConfigurationContainer (as discussed in #18624 and #15362)

Suggested API

public ContainerConfiguration WithInstance<TExport> (TExport instance);
public ContainerConfiguration WithInstance<TExport> (string contractName, TExport instance);
public ContainerConfiguration WithInstance (Type t, object instance);
public ContainerConfiguration WithInstance (Type t, string contractName, object instance);

(Thanks to @NEKIT-Boss for the examples in #18624)

Usage

ContainerConfiguration containerConfig = new ContainerConfiguration()
    .WithAssembly(GetType().Assembly)
    .WithInstance<IExample>(new Example());

var container = containerConfig.CreateContainer();
var example = container.GetExport<IExample>();
@danmoseley
Copy link
Member

Unfortunately we have no real expertise in MEF remaining on the team. @weshaggard did you work on this code?

@weshaggard
Copy link
Member

I'v reviewed some of the MEF2 stuff but I didn't code it. I did however code the MEF1 stuff many years ago and from what I know of MEF2 the APIs seems like they would be reasonable but I would suggest someone doing a proof of concept implementation of them before formally accepting the API proposal. The reason for that is to ensure that these fit into the model that MEF2 uses.

@danmoseley
Copy link
Member

Thanks @weshaggard . Does anyone around have knowledge of MEF2 specifically?

@weshaggard
Copy link
Member

I doubt there is anyone that has any recent knowledge but @nblumhardt wrote it originally and might have some memory of the code.

@nblumhardt
Copy link

Hey @danmosemsft @weshaggard @gthvidsten !

This is a pretty straightforward addition; I wrote and included it as an extension WithExport() in the original MEF2 Code drop, along with another useful one, WithFactoryDelegate():

var config = new ContainerConfiguration()
    .WithExport<TextWriter>(Console.Out)
    .WithFactoryDelegate<string>(() => DateTime.Now.ToString(), isShared: true)
    .WithPart<Program>();

The terminology WithExport() could be the some bikeshedding but as far as I remember it's the most accurate WRT the underlying model.

The demo project is here: https://github.com/MicrosoftArchive/mef/blob/master/oob/demo/Microsoft.Composition.Demos.ExtendedPartTypes/Program.cs#L24

The extension method WithExport() just adds an InstanceExportDescriptorProvider to the container:

namespace Microsoft.Composition.Demos.ExtendedPartTypes.Extension
{
    static class ContainerConfigurationExtensions
    {
        public static ContainerConfiguration WithExport<T>(this ContainerConfiguration configuration, T exportedInstance, string contractName = null, IDictionary<string, object> metadata = null)
        {
            return WithExport(configuration, exportedInstance, typeof(T), contractName, metadata);
        }

        public static ContainerConfiguration WithExport(this ContainerConfiguration configuration, object exportedInstance, Type contractType, string contractName = null, IDictionary<string, object> metadata = null)
        {
            return configuration.WithProvider(new InstanceExportDescriptorProvider(
                exportedInstance, contractType, contractName, metadata));
        }

And the InstanceExportDescriptorProvider itself is pretty straightforward:

    // we're just aiming to show the mechanics here.
    class InstanceExportDescriptorProvider : SinglePartExportDescriptorProvider
    {
        object _exportedInstance;

        public InstanceExportDescriptorProvider(object exportedInstance, Type contractType, string contractName, IDictionary<string, object> metadata)
            : base (contractType, contractName, metadata)
        {
            if (exportedInstance == null) throw new ArgumentNullException("exportedInstance");
            _exportedInstance = exportedInstance;
        }

        public override IEnumerable<ExportDescriptorPromise> GetExportDescriptors(CompositionContract contract, DependencyAccessor descriptorAccessor)
        {
            if (IsSupportedContract(contract))
                yield return new ExportDescriptorPromise(contract, _exportedInstance.ToString(), true, NoDependencies, _ =>
                    ExportDescriptor.Create((c, o) => _exportedInstance, Metadata));
        }
    }

Since it's all based on the public API/extension points, you can drop this in today @gthvidsten and it should "just work".

It'd be wonderful to see this added to the official package, or included in the docs somewhere, if anyone picks it up (it still pains me that there's so much great stuff buried in this little project).

@bryanchen-d
Copy link

+1 vote for this feature.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@ericstj ericstj added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Jul 9, 2020
@ericstj
Copy link
Member

ericstj commented Jul 9, 2020

Thanks for the input @nblumhardt, I've gone ahead and marked this one ready-for-review. Cheers!

@terrajobst
Copy link
Member

terrajobst commented Jul 9, 2020

I'd go with @nblumhardt's proposal, slightly adjusted:

namespace System.Composition.Hosting
{
    public class ContainerConfiguration
    {
        public ContainerConfiguration WithExport<TExport>(TExport exportedInstance);
        public ContainerConfiguration WithExport<TExport>(TExport exportedInstance, string? contractName = null, IDictionary<string, object>? metadata = null);

        public ContainerConfiguration WithExport(Type contractType, object exportedInstance);
        public ContainerConfiguration WithExport(Type contractType, object exportedInstance, string? contractName = null, IDictionary<string, object>? metadata = null);
    }
}  

@bartonjs
Copy link
Member

bartonjs commented Jul 9, 2020

Video

After some discussion on the name of the methods, the use of defaults parameters, and the name of the generic, it's approved as

namespace System.Composition.Hosting
{
    public class ContainerConfiguration
    {
        public ContainerConfiguration WithExport<TExport>(TExport exportedInstance);
        public ContainerConfiguration WithExport<TExport>(TExport exportedInstance, string? contractName = null, IDictionary<string, object>? metadata = null);

        public ContainerConfiguration WithExport(Type contractType, object exportedInstance);
        public ContainerConfiguration WithExport(Type contractType, object exportedInstance, string? contractName = null, IDictionary<string, object>? metadata = null);
    }
} 

We also think it would be valueable to add an analyzer that warns when generic inference was used. (e.g. WithExport(new SomeExportedType()))

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 9, 2020
@ericstj
Copy link
Member

ericstj commented Jul 29, 2020

@gthvidsten @nblumhardt this API was approved, were either of you going to drive the implementation in? If we want this in .NET 5.0 it would need to be merged before 8/18 or sooner.

@maryamariyan maryamariyan added help wanted [up-for-grabs] Good issue for external contributors and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 29, 2020
@nblumhardt
Copy link

I may get a chance - a bit short of time, though, so I'll be pleased if someone else beats me to it :-)

@danmoseley danmoseley modified the milestones: 5.0.0, Future Jul 31, 2020
@danmoseley
Copy link
Member

Setting milestone to indicate this is not a required part of the 5.0 product.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 8, 2022
@tarekgh tarekgh modified the milestones: Future, 7.0.0 Mar 16, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 16, 2022
@ZigMeowNyan
Copy link

Should also close issue #27324.

As the person who originally opened #15362 back in 2015, I'm glad to see this finally make it into the official packages. It drastically simplifies migration from MEF1 to MEF2 for people who don't need the flexibility of MEF1 and want to avoid the potentially higher performance cost. Refactoring a project to use plugins is a lot easier when you don't have to fully rearchitect to fit design assumptions and can just export an existing instance.

I'll be glad to review my legacy projects and adjust for this. Might even have time to swap out the old handwritten context classes for AssemblyLoadContext.

@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Composition enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
No open projects
Development

Successfully merging a pull request may close this issue.