Remove asynchrony from service registration#79493
Conversation
| await JoinableTaskFactory.SwitchToMainThreadAsync(ct); | ||
| return new TempPECompilerService(workspace.Services.GetService<IMetadataService>()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
@jasonmalinowski i can also take this further. If we're just registering a callback for some future point, then no need to have this as a task on PackageInitializationBackgroundThreadAsync. We can just early register this.
Note: i don't know this part of the system super well. so LMK if that's undesirable for some reason.
There was a problem hiding this comment.
I'd say let's just leave that as is for now; that way everything is going through the registration system we have rather than a mix of things.
Ideally, I'd like to be to a point where we can delete all the fancy registration stuff, not because it's bad, but hopefully we're to the point where we have so little in our package startup that we don't really need the fanciness again. But we're not remotely there yet.
| Function(_1, _2) New TempPECompilerFactory(Me.ComponentModel.GetService(Of VisualStudioWorkspace)())) | ||
| Catch ex As Exception When FatalError.ReportAndPropagateUnlessCanceled(ex) | ||
| Throw ExceptionUtilities.Unreachable | ||
| End Try |
There was a problem hiding this comment.
same thing here. this can all avoid this packageinitializetasks work if not needed.
There was a problem hiding this comment.
Ditto: let's leave it for consistency, we'll switch over once we have things cleaned up a lot more.
jasonmalinowski
left a comment
There was a problem hiding this comment.
This service is already being registered in src\VisualStudio\CSharp\Impl\PackageRegistration.pkgdef; we should edit that (or delete it) rather than adding a registration in code as one will win and it's not well defined who.
|
I'm a fan of code, so i'll delete hte pkgdef part :) |
Fine with me; I'm a bit confused on the source control history here though. IIRC we had gotten rid of the code definitions because the SDK wasn't working well with dotnet builds, but I see that is set. @tmat any idea what the intent was here? |
Fixes #79395