-
Notifications
You must be signed in to change notification settings - Fork 703
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
WIP: update package versions to use resolved version #1665
Conversation
@emgarten @jainaashish still a WIP, could you please give me feedback if this approach is the way to go or if there is a better way to achieve what I'm trying to do. Thanks! |
@@ -137,6 +142,7 @@ private PackageSourceMoniker SelectedSource | |||
|
|||
Loaded += (_, __) => | |||
{ | |||
ReadAssetsFileAndUpdateDependencyLookup(); |
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.
reading assets file and updating dependency lookup should be part of search task which will make sure it doesn't delay the ui load.
|
||
_installedPackagesTask = PackageCollection.FromProjectsAsync(Projects, CancellationToken.None); | ||
_installedPackagesTask = PackageCollection.FromProjectsAsync(Projects, null, CancellationToken.None); | ||
_resolvedPackagesTask = PackageCollection.FromProjectsAsync(Projects, DependencyVersionLookup, CancellationToken.None); |
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 do you really need two different package collections? I would expected it to be single resolved packages
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.
At first I only had one package collection, but the consolidate tab relies on the installed packages collection and when I used the resolved packages for this particular tab it didn't work. That's why I decided to do two package collections, any idea how this issue could be approached?
|
||
namespace NuGet.PackageManagement.VisualStudio | ||
{ | ||
public class NuGetProjectDependencyVersionLookup |
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 make it specific to build integrated projects? You can keep it generic to have all the resolved versions for each package across any kind of project. Just that in case of build integrated it will read from assets file vs for packages.config, it will be from config file.
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.
What the client does right now is to load the installed package identities from the config file (either project.json or packages.config, or csproj) and use them as a base for loading the UI. (Maybe I'm understanding something incorrectly?) What I'm trying to accomplish is updating those already loaded references with the resolved version. If this type is not specific to build integrated projects it would read the config file again (for packages.config projects) and try to update the values with the ones it already has.
At first I thought it would be a good idea to make this change from where the identities are loaded (for example BuildIntegratedPackageReference:54
), but it seemed that going that direction was a more involved change on how package references are built for this type of projects.
5d1fc22
to
9415900
Compare
This looks like an old PR. This is a soft reminder to clean it up or close it until such time as it is active again. |
Fixes NuGet/Home#3788, NuGet/Home#3101 & NuGet/Home#3109