Enable automatic nuget restore on the client #6676
Conversation
dibarbet
force-pushed
the
automatic_nuget_restore
branch
from
edb2d9e
to
afb4325
Compare
src/lsptoolshost/restore.ts
Outdated
| }) | ||
| ); | ||
|
|
||
| languageServer.registerOnRequest(UnresolvedProjectDependenciesNotification.type, async (params) => { |
There was a problem hiding this comment.
Is this a request or a notification? Or I guess the underlying type is a void which means notification?
There was a problem hiding this comment.
Its a request that returns void (but waits until completion) - there are separate APIs for notifications
| languageServer: RoslynLanguageServer, | ||
| restoreChannel: vscode.OutputChannel, | ||
| projectFile?: string | ||
| projectFiles: string[], | ||
| showOutput: boolean | ||
| ): Promise<void> { | ||
| if (_restoreInProgress) { | ||
| vscode.window.showErrorMessage(vscode.l10n.t('Restore already in progress')); |
There was a problem hiding this comment.
Is there a possibility we can now hit this if automatic restore is on? I'm thinking something like the user invokes restore, it starts to restore projects, and tries (but fails) to fully restore one project, call it A. It's still going and marching to restore other projects in the solution, but we saw the restore of A, and then trigger a reload which triggers a check of A's dependencies. We see it's missing, and so we trigger a restore of A again, but the restore of the rest of the solution is still going on...
Should we be queueing these instead in some way? That might also make sense if somebody is invoking two project restores one after the other in rapid succession.
There was a problem hiding this comment.
This should be safe on the reload side because the restores on the client will block the project load queue until they're done. So the reload won't be processed until the rest of the projects finish restoring (I actually hit this issue with the first iteration with notifications)
This should only get hit if someone triggers a manual restore while an automatic one is in progress
There was a problem hiding this comment.
Yeah manual and automatic restore happening at once was what worried me.
There was a problem hiding this comment.
Yeah it is possible to hit this with a manual restore. I think for now though we don't need a queue - it should be a relatively uncommon case. If its not and ends up being a bad experience for people we can come back and add it.
There was a problem hiding this comment.
@dibarbet Maybe this is better to defer anyways until we get all the restores being done by the server rather thant he back-and-forth happening here anyways.
dibarbet
force-pushed
the
automatic_nuget_restore
branch
from
afb4325
to
1fc7f22
Compare
dibarbet
force-pushed
the
automatic_nuget_restore
branch
from
1fc7f22
to
4f20608
Compare
| /** | ||
| * The set of projects that have unresolved dependencies and require a restore. | ||
| */ |
There was a problem hiding this comment.
Might still need a rename or update too since we're also handling other changes.
src/lsptoolshost/roslynProtocol.ts
Outdated
| @@ -260,3 +267,9 @@ export namespace RestorableProjects { | |||
| export const messageDirection: lsp.MessageDirection = lsp.MessageDirection.clientToServer; | |||
| export const type = new lsp.RequestType0<string[], void>(method); | |||
| } | |||
|
|
|||
| export namespace ProjectHasUnresolvedDependenciesNotification { | |||
There was a problem hiding this comment.
| export namespace ProjectHasUnresolvedDependenciesNotification { | |
| export namespace ProjectNeedsRestoreRequest { |
There was a problem hiding this comment.
"Request" seems most critical if the API pattern distinguishes request vs. notification to avoid other confusion, so if you want to go with ProjectHasUnresolvedDependenciesRequest and then you want to mop-up other renames later, that's fine too.
There was a problem hiding this comment.
changed to request. if I'm changing the underlying method name, it will need a server update. I'll do all that separately.
TODO - Requires server side update as wellServer side - https://dev.azure.com/dnceng/internal/_build/results?buildId=2329040&view=results
See dotnet/roslyn#70851 for details