-
Notifications
You must be signed in to change notification settings - Fork 336
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
fix: ensure that npm dependencies retain their "production" grouping #939
Changes from all commits
3c68c3d
d2be869
53f080c
e4b7862
b8712e5
1ad64e2
1ec3349
7eb2102
b2f5ce0
c132a55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
{ | ||
"requires": true, | ||
"lockfileVersion": 1, | ||
"dependencies": { | ||
"ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"dev": true, | ||
"requires": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
}, | ||
"eslint": { | ||
"version": "1.2.3", | ||
"dev": true, | ||
"dependencies": { | ||
"ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"dev": true, | ||
"requires": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
}, | ||
"dependencies": { | ||
"ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"optional": true, | ||
"requires": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
}, | ||
"table": { | ||
"version": "1.0.0", | ||
"dependencies": { | ||
"ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"requires": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
}, | ||
"dependencies": { | ||
"ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"dev": true, | ||
"optional": true, | ||
"requires": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
{ | ||
"name": "my-library", | ||
"lockfileVersion": 2, | ||
"requires": true, | ||
"packages": { | ||
"": { | ||
"dependencies": {}, | ||
"devDependencies": {} | ||
}, | ||
"node_modules/ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"dev": true, | ||
"dependencies": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
}, | ||
"node_modules/eslint": { | ||
"version": "1.2.3", | ||
"dev": true | ||
}, | ||
"node_modules/eslint/node_modules/ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"dev": true, | ||
"dependencies": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
}, | ||
"node_modules/eslint/node_modules/ajv/node_modules/ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"optional": true, | ||
"dependencies": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
}, | ||
"node_modules/table": { | ||
"version": "1.0.0" | ||
}, | ||
"node_modules/table/node_modules/ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"dependencies": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
}, | ||
"node_modules/table/node_modules/ajv/node_modules/ajv": { | ||
"version": "5.5.2", | ||
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz", | ||
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==", | ||
"devOptional": true, | ||
"dependencies": { | ||
"co": "^4.6.0", | ||
"fast-deep-equal": "^1.0.0", | ||
"fast-json-stable-stringify": "^2.0.0", | ||
"json-schema-traverse": "^0.3.0" | ||
} | ||
} | ||
}, | ||
"dependencies": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
"strings" | ||
|
||
"golang.org/x/exp/maps" | ||
"golang.org/x/exp/slices" | ||
) | ||
|
||
type NpmLockDependency struct { | ||
|
@@ -49,6 +50,39 @@ type NpmLockfile struct { | |
|
||
const NpmEcosystem Ecosystem = "npm" | ||
|
||
type npmPackageDetailsMap map[string]PackageDetails | ||
|
||
// mergeNpmDepsGroups handles merging the dependency groups of packages within the | ||
// NPM ecosystem, since they can appear multiple times in the same dependency tree | ||
// | ||
// the merge happens almost as you'd expect, except that if either given packages | ||
// belong to no groups, then that is the result since it indicates the package | ||
// is implicitly a production dependency. | ||
func mergeNpmDepsGroups(a, b PackageDetails) []string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems we only need
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could but that'd mean every caller of this function would have to explicitly access |
||
// if either group includes no groups, then the package is in the "production" group | ||
if len(a.DepGroups) == 0 || len(b.DepGroups) == 0 { | ||
return nil | ||
} | ||
|
||
combined := make([]string, 0, len(a.DepGroups)+len(b.DepGroups)) | ||
combined = append(combined, a.DepGroups...) | ||
combined = append(combined, b.DepGroups...) | ||
|
||
slices.Sort(combined) | ||
|
||
return slices.Compact(combined) | ||
} | ||
|
||
func (pdm npmPackageDetailsMap) add(key string, details PackageDetails) { | ||
existing, ok := pdm[key] | ||
|
||
if ok { | ||
details.DepGroups = mergeNpmDepsGroups(existing, details) | ||
} | ||
|
||
pdm[key] = details | ||
} | ||
|
||
func (dep NpmLockDependency) depGroups() []string { | ||
if dep.Dev && dep.Optional { | ||
return []string{"dev", "optional"} | ||
|
@@ -64,7 +98,7 @@ func (dep NpmLockDependency) depGroups() []string { | |
} | ||
|
||
func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[string]PackageDetails { | ||
details := map[string]PackageDetails{} | ||
details := npmPackageDetailsMap{} | ||
|
||
for name, detail := range dependencies { | ||
if detail.Dependencies != nil { | ||
|
@@ -98,14 +132,14 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str | |
} | ||
} | ||
|
||
details[name+"@"+version] = PackageDetails{ | ||
details.add(name+"@"+version, PackageDetails{ | ||
Name: name, | ||
Version: finalVersion, | ||
Ecosystem: NpmEcosystem, | ||
CompareAs: NpmEcosystem, | ||
Commit: commit, | ||
DepGroups: detail.depGroups(), | ||
} | ||
}) | ||
} | ||
|
||
return details | ||
|
@@ -137,7 +171,7 @@ func (pkg NpmLockPackage) depGroups() []string { | |
} | ||
|
||
func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]PackageDetails { | ||
details := map[string]PackageDetails{} | ||
details := npmPackageDetailsMap{} | ||
|
||
for namePath, detail := range packages { | ||
if namePath == "" { | ||
|
@@ -159,14 +193,14 @@ func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]Package | |
finalVersion = commit | ||
} | ||
|
||
details[finalName+"@"+finalVersion] = PackageDetails{ | ||
details.add(finalName+"@"+finalVersion, PackageDetails{ | ||
Name: finalName, | ||
Version: detail.Version, | ||
Ecosystem: NpmEcosystem, | ||
CompareAs: NpmEcosystem, | ||
Commit: commit, | ||
DepGroups: detail.depGroups(), | ||
} | ||
}) | ||
} | ||
|
||
return details | ||
|
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.
maybe add a comment stating that the key is name and version