-
Notifications
You must be signed in to change notification settings - Fork 407
Update MicrosoftDotNetHotReloadPackageVersion and CPSPackageVersion #9802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…o latest pre-release versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some questions about the utility class. Was it added by accident? Seems unrelated to this PR, so easier to just remove it for now, unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add this without any apparent usages? It's fine to add things like this, but better when they're actually used at the time they're added, so that we can tell it adds value to the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required by the newer version of Microsoft.DotNet.HotReload.Client source package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, HotReloadClients
requires that extenion. It's from a source package so the change is not in the git history
public ValueTask ReportCompilationErrorsInApplicationAsync(ImmutableArray<string> compilationErrors, CancellationToken cancellationToken)
=> browserRefreshServer?.ReportCompilationErrorsInBrowserAsync(compilationErrors, cancellationToken) ?? ValueTask.CompletedTask;

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also update PolySharp to generate this extension code. For now I'd go with this change since it's simpler.
src/Microsoft.VisualStudio.ProjectSystem.Managed/ValueTaskExtension.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.VisualStudio.ProjectSystem.Managed/ValueTaskExtension.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.VisualStudio.ProjectSystem.Managed/ValueTaskExtension.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.VisualStudio.ProjectSystem.Managed/ValueTaskExtension.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.VisualStudio.ProjectSystem.Managed/ValueTaskExtension.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.VisualStudio.ProjectSystem.Managed/ValueTaskExtension.cs
Outdated
Show resolved
Hide resolved
… and unnecessary preprocessor directives
|
||
namespace System.Threading.Tasks; | ||
|
||
internal static partial class ValueTaskExtensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why partial
?
…o latest pre-release versions