Skip to content

Dev zhi project provider async service - #2163

Merged
zhili1208 merged 1 commit into
devfrom
dev-zhi-ProjectProviderAsyncService
Apr 20, 2018
Merged

zhili1208 merged 1 commit into
devfrom
dev-zhi-ProjectProviderAsyncService

Conversation

@zhili1208

@zhili1208 zhili1208 commented Apr 16, 2018

Copy link
Copy Markdown
Contributor

using AsyncServiceProvider in VSCredentialServiceProvider and all ProjectProvider.

NuGet/Home#6771


[ImportingConstructor]
internal DefaultVSCredentialServiceProvider(Lazy<INuGetUILogger> outputConsoleLogger)
: this(AsyncServiceProvider.GlobalProvider, outputConsoleLogger)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this AsyncServiceProvider.GlobalProvider and is it MEF component? how is this being resolved?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a MEF component, it's a static property which returns AsyncServiceProvider

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's a static property then why it's part of MEF constructor?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not in the MEF constructor, it's passed to this() after MEF constructing done.

IServiceProvider serviceProvider,
Lazy<INuGetUILogger> outputConsoleLogger
IAsyncServiceProvider asyncServiceProvider,
Lazy<INuGetUILogger> outputConsoleLogger

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still needed? are we using it in tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really needed now, just keep this pattern, in case we need to pass different AsyncServiceProvider in the future.

public NuGet.Configuration.ICredentialService GetCredentialService()
public async Task<NuGet.Configuration.ICredentialService> GetCredentialServiceAsync()
{
ThreadHelper.ThrowIfNotOnUIThread();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it still need to be on UI thread?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be ok on worker thread, I can remove this

threadingService)
{ }

public LegacyPackageReferenceProjectProvider(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above do we still need this constructor?

{
private readonly IVsProjectThreadingService _threadingService;
private readonly Lazy<IComponentModel> _componentModel;
private readonly AsyncLazy<IComponentModel> _componentModel;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jainaashish jainaashish left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhili1208
zhili1208 force-pushed the dev-zhi-ProjectProviderAsyncService branch from 0ece978 to 0e2fe42 Compare April 19, 2018 23:03
@zhili1208
zhili1208 force-pushed the dev-zhi-ProjectProviderAsyncService branch from 38b0cec to 743acaf Compare April 20, 2018 17:17
@zhili1208
zhili1208 merged commit 74bd571 into dev Apr 20, 2018
@zhili1208
zhili1208 deleted the dev-zhi-ProjectProviderAsyncService branch July 11, 2018 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants