search

NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

`NuGetSearchServiceReconnector` should be evaluated for refactoring #12722

Open donnie-msft opened 1 year ago

donnie-msft commented 1 year ago

The NuGetSearchServiceReconnector has a structure where it looks at a wrapped object and then its service. Evaluate this pattern for refactoring.

https://github.com/NuGet/NuGet.Client/blob/2c0808892b2debe1a48fd0a4af644d809598023b/src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/NuGetSearchServiceReconnector.cs#L119

https://github.com/NuGet/NuGet.Client/pull/5288/files/3cce587937a4bc400073d92c43685c3b091d4518#r1248132907

/cc @zivkan

zivkan commented 1 year ago

Brokered services were designed to allow connections to other processes, either out-of-process on the same machine, or a remote machine, for example over Live Share. I don't believe PM UI actually works over Live Share, but these brokered services could enable the possibility if we wanted to put in the effort.

When connecting to remote machines, the network connection can be lost. Hence code using brokered services need to handle when the connection is lost. This class handles disconnections by automatically reconnecting (not exactly a safe assumption, if the network is down, or the other machine is no longer reachable).

However, all of that is theoretical, not practical, because as mentioned in the first paragraph, while some of this architecture was put in place, we're neither running out of process, or supporting Live Share.

donnie-msft commented 1 year ago

When connecting to remote machines, the network connection can be lost. Hence code using brokered services need to handle when the connection is lost. This class handles disconnections by automatically reconnecting (not exactly a safe assumption, if the network is down, or the other machine is no longer reachable).

This issue suggested refactoring the class. Are you interpreting that as removing the class?

zivkan commented 1 year ago

I was proving more context, in case anyone reading this issue isn't aware. Hopefully my comment is quicker to understand than reading the entire source file and figuring it out.