From 399c409b7008f5b854106b4ec3142674c0f9ce03 Mon Sep 17 00:00:00 2001 From: Michael Kedar Date: Tue, 21 Jan 2025 13:58:07 +1100 Subject: [PATCH] fix(guided remediation): npm workspaces devDeps in in-place strategy (#1512) `devDependencies` can be declared in a workspace, which is not a direct dependency (`root` -> `workspace` -> `dev`). Made the IsDev check that `in-place` uses also check these so that `--ignore-dev` works properly for `in-place` with npm workspace. (`--ignore-dev` still doesn't behave correctly for `relax` with workspaces, but relax doesn't really work with workspaces anyway) Also fixed a potential panic with in-place & private registries that don't behave as expected w.r.t. `optionalDependencies` being in regular `dependencies` --- internal/remediation/in_place.go | 6 ++++-- internal/resolution/dependency_chain.go | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/remediation/in_place.go b/internal/remediation/in_place.go index f7547723eb2..16d2a5fa7d0 100644 --- a/internal/remediation/in_place.go +++ b/internal/remediation/in_place.go @@ -342,11 +342,13 @@ func dependenciesSatisfied(ctx context.Context, cl client.DependencyClient, vk r } // TODO: correctly handle other attrs e.g. npm peerDependencies - // remove the optional deps from the regular deps (because they show up in both) if they're not already installed + // remove the optional deps from the regular deps (because they can show up in both) if they're not already installed for _, optVk := range optDeps { if !slices.ContainsFunc(children, func(vk resolve.VersionKey) bool { return vk.Name == optVk.Name }) { idx := slices.IndexFunc(deps, func(vk resolve.VersionKey) bool { return vk.Name == optVk.Name }) - deps = slices.Delete(deps, idx, idx+1) + if idx >= 0 { + deps = slices.Delete(deps, idx, idx+1) + } } } diff --git a/internal/resolution/dependency_chain.go b/internal/resolution/dependency_chain.go index 63f73cdd07c..0b89ab19e9e 100644 --- a/internal/resolution/dependency_chain.go +++ b/internal/resolution/dependency_chain.go @@ -38,11 +38,15 @@ func (dc DependencyChain) End() (resolve.VersionKey, string) { func ChainIsDev(dc DependencyChain, groups map[manifest.RequirementKey][]string) bool { edge := dc.Edges[len(dc.Edges)-1] // This check only applies to the graphs created from the in-place lockfile scanning. - // TODO: consider dev dependencies in e.g. workspaces that aren't direct if edge.Type.HasAttr(dep.Dev) { return true } + // As a workaround for npm workspaces, repeat above in-place check 1 layer deeper. + if len(dc.Edges) > 1 && dc.Edges[len(dc.Edges)-2].Type.HasAttr(dep.Dev) { + return true + } + req := resolve.RequirementVersion{ VersionKey: dc.Graph.Nodes[edge.To].Version, Type: edge.Type.Clone(), @@ -51,7 +55,7 @@ func ChainIsDev(dc DependencyChain, groups map[manifest.RequirementKey][]string) if !ok { return false } - + // TODO: Below check doesn't support npm workspaces correctly. return lockfile.Ecosystem(ecosystem).IsDevGroup(groups[manifest.MakeRequirementKey(req)]) }