Teach OOP how to MEF #9953
Teach OOP how to MEF #9953
Conversation
| [Export(typeof(ITelemetryReporter)), Shared] | ||
| internal class OutOfProcessTelemetryReporter : TelemetryReporter | ||
| { | ||
| public OutOfProcessTelemetryReporter() |
There was a problem hiding this comment.
lol, I did that originally but it looked too short. Felt better to let the code breathe a bit. Happy to take a vote though, internal class OutOfProcessTelemetryReporter() : TelemetryReporter([TelemetryService.DefaultSession]); totally would work.
There was a problem hiding this comment.
I'm fine either way. To be quite honest, I'm not a huge fan of primary constructors so far, especially since we are still using separately defined fields with underscore prefix, which seems to defeat some of the purpose.
| using System.Collections.Immutable; | ||
| using System.Reflection; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.VisualStudio.Composition; |
There was a problem hiding this comment.
Is Roslyn OOP using VS MEF? If not, should we be pulling that in? Or, is MEFv2 sufficient?
There was a problem hiding this comment.
To be clear, I would have expected us to use CompositionHost rather than the VS ExportProvider.
There was a problem hiding this comment.
Roslyn does use VS MEF, but I'm happy to change to using v2 directly if that is preferred. I was just copying code from Roslyn because its easier than working out how to write it myself (though perhaps the subsequent debugging to see where it went wrong actually took more time? 😛)
There was a problem hiding this comment.
Got it. If Roslyn's using VS MEF, then I have no concerns. :)
|
Thanks for the reviews. Have cherry-picked in the integration test fix, so I can run the integration tests, and will merge if they pass. |
I'm mainly putting this up so @DustinCampbell can tell me what I did wrong :)
As part of cohosting we're going to need a lot more things to run in OOP, so its probably best to move away from these hand-constructed services that it has. This PR introduced MEF, in a reasonably unsophisticated way, into the procedure so that services can at least get some things from an
ExportProvider. Once in that world, obviously everything works as expected with attributes etc.Along the way I also found that our telemetry reporting in OOP wasn't doing anything, so fixed that and moved it to the MEF composition too. Also improved the
launchSettings.jsonfile a little, and found it was previously in the.gitignore. This seemed wrong to me, but maybe @dotnet/razor-compiler will have something different to say about that? I can always move the compiler one back in if necessary.Part of #9519