interactive icon indicating copy to clipboard operation
interactive copied to clipboard
verified publisher icon dotnet

NamedPipeConnector Fixes

Open michael-hawker opened this issue 1 year ago • 2 comments

Contributes to #3720 (not sure why it's not uploaded to NuGet, but noticed some issues while investigating the WPF Connect sample, so figured I'd push these up as a separate PR here so it'd be more ready at least!)

This PR does 3 things:

  • Use straight net8.0 as the TFM.
    • The net8.0-windows TFM, I think is the old way from .NET 5 only? Generally, it should have a version number after it for targeting modern Windows, this happened in .NET 6, see https://learn.microsoft.com/windows/apps/desktop/modernize/desktop-to-uwp-enhance
    • Regardless, from what I can tell anyway, the NamedPipeClientStream which is the main part of this code is still just a pure .NET API and doesn't need the Windows specific TFM https://learn.microsoft.com/dotnet/api/system.io.pipes.namedpipeclientstream
    • Having a windows TFM breaks the connect-wpf sample as the notebook in VS Code can't load the windows TFM (I have a draft PR to update the connect-wpf sample, and I think it can connect now with some help, but stalls out, so I think I'm stuck, but this was the first step, so figured would make it an independent PR as the wpf sample is pinned in the past anyway.)
  • Update the Package name to match the project name (extra Connector was on there for some reason, copy/paste bug?)
    • Looked later, this may be the root cause of #3720 and the package missing on NuGet as the publish script (see comment below) looks for the expected name
  • When loading this extension in a notebook, it would fail as the code in the extension.dib wasn't updated alongside the other code in this project
    • This is where I had to guess a bit, but tried to pattern off the SQL Lite extension, and just created a new static method to add the directive to the root when the extension is initialized.

michael-hawker avatar Oct 18 '24 19:10 michael-hawker

Thanks, @michael-hawker!

jonsequitur avatar Oct 18 '24 19:10 jonsequitur

@jonsequitur my pleasure, thanks for the quick approval! Was going to continue the discussion in #3720, but then realized maybe this PR will fix that issue too... Looking here:

https://github.com/dotnet/interactive/blob/1f11b72e79680e17fba039cfa02aea112645bf01/eng/publish/PublishVSCodeExtension.ps1#L50

It seems like it's part of the publish list, but was using the csproj name (as expected), so since the package name was wrong it was just probably missed... 😅

Now that the name is fixed, maybe it should be picked up on your next publish?

michael-hawker avatar Oct 18 '24 20:10 michael-hawker