Skip to content

Commit

Permalink
fix(guided remediation): npm workspaces devDeps in in-place strategy (#…
Browse files Browse the repository at this point in the history
…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`
  • Loading branch information
michaelkedar authored Jan 21, 2025
1 parent 4fab586 commit 399c409
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 4 deletions.
6 changes: 4 additions & 2 deletions internal/remediation/in_place.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}

Expand Down
8 changes: 6 additions & 2 deletions internal/resolution/dependency_chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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)])
}

Expand Down

0 comments on commit 399c409

Please sign in to comment.