madskristensen / Community.VisualStudio.Toolkit

A community toolkit for writing Visual Studio extensions
Other
24 stars 3 forks source link

Added VS.Initialize and VS.JoinableTaskFactory #24

Open bluetarpmedia opened 3 years ago

bluetarpmedia commented 3 years ago

VS.Initialize is a new, optional method that extension authors can (preferably should) call during their AsyncPackage's InitializeAsync, and pass in their AsyncPackage's JoinableTaskFactory instance.

By default, the VS.JoinableTaskFactory property returns the ThreadHelper.JoinableTaskFactory instance.

But if the extension calls VS.Initialize then VS.JoinableTaskFactory will subsequently return the extension AsyncPackage's JTF instance.

Classes in the Community.VisualStudio.Toolkit should use VS.JoinableTaskFactory.RunAsync instead of ThreadHelper.JoinableTaskFactory.RunAsync for the reasons outlined in https://github.com/madskristensen/Community.VisualStudio.Toolkit/issues/7

As a result, if an extension uses the Community.VisualStudio.Toolkit and chooses to call VS.Initialize then it will benefit from the task tracking that occurs with their extension package's JTF instance, because all async work started by the Toolkit will use their extension's JTF instance.

madskristensen commented 3 years ago

Great idea, but a few concerns/ideas.

Having a static reference from the VS class might be problematic when you have more than one extension installed using the save version of the Community.VisualStudio.Toolkit.

@reduckted is working on implementing a TookitPackage base class to use instead of AsyncPackage. It could initialize the toolkit by passing on the JTF behind the scenes, so the user doesn't have to remember to do it.

bluetarpmedia commented 3 years ago

@madskristensen @reduckted Sounds great, yes that'll be much better than my approach.

How will methods throughout the Toolkit get access to the package's JTF instance? E.g. TaskExtensions.FireAndForget.

reduckted commented 3 years ago

@bluetarpmedia I don't have any answers to your questions (it sounds like those extension methods are going to be tricky), but I will throw this into the mix regarding unit testing: https://github.com/microsoft/vs-threading/blob/fad0c9a9fe47ae0678c81f6ffd253b7a02d21b93/doc/testing_vs.md

The relevant part:

The recommended pattern for testability is that all your code refer to a YourPackage.JoinableTaskFactory property instead of ThreadHelper directly. The YourPackage.Initialize method should initialize this property to ThreadHelper.JoinableTaskFactory.

It sounds like that's along the lines of what you're trying to achieve, which is awesome. I just don't know how the extension methods that are package-independent would access the JTF.

reduckted commented 3 years ago

Having a static reference from the VS class might be problematic when you have more than one extension installed using the save version of the Community.VisualStudio.Toolkit.

What if you walked up the call stack to find which assembly the JTF was requested from? Something like this: https://github.com/madskristensen/Community.VisualStudio.Toolkit/compare/e5b33a26221238af94b7bfaf277e7e64d5a5c8f5...reduckted:prototype%2Fuse-package-jtf

I don't think the performance impact is anything to worry about, especially since we wouldn't need to get any file info from the stack trace. From what I understand, passing fNeedFileInfo: true to the StackTrace constructor is what would cause poor performance.

madskristensen commented 3 years ago

This feels a little bit like a solution looking for a problem to me. I've argued in the past for not doing unit test of code with VS dependencies, but instead argued for cleaner architectural separation and have functional or integration tests for the VS parts only. Mocking out VS dependencies is just a hazzle and I'm not sure you're winning much by doing it. It's the functional tests that really matter for the VS layer IMO. Does the ThreadHelper complicate/block the testability of your extensions today?

For MEF extensions without a package class for example, using the ThreadHelper is the only option. I assume that's why it exists in the first place and is being used by VS itself.