Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Commit

Permalink
fix(shrinkwrap): always set name on the root node, closes npm/cli#2264
Browse files Browse the repository at this point in the history 
PR-URL: #310
Credit: @nlf
Close: #310
Reviewed-by: @isaacs
  • Loading branch information
@nlf @isaacs
nlf authored and isaacs committed Aug 12, 2021
1 parent 77e279a commit 32cdebb
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
6 changes: 4 additions & 2 deletions lib/shrinkwrap.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,11 @@ class Shrinkwrap {
if (val)
meta[key.replace(/^_/, '')] = val
})
// we only include name if different from the node path name
// we only include name if different from the node path name, and for the
// root to help prevent churn based on the name of the directory the

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Aug 12, 2021

Contributor

ideally it wouldn't depend on the directory name at all, since that often varies across machines

This comment has been minimized.

Sign in to view

Copy link
@nlf

nlf Aug 12, 2021

Author Contributor

correct, that's exactly what this is fixing. we will now unconditionally put a 'name' field on the root node. we still only put a name property on child nodes when the directory name differs from the package name to save bytes in the package-lock.json, since within the structure of node_modules the directory name almost always matches the package name. the root node is the one that most often differs between machines

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Aug 12, 2021

Contributor

ah, so the initial name would be nondeterministic, but future ones would remain unchanged. that's at least an improvement :-)

seems better tho to never look at the directory name in the first place.

This comment has been minimized.

Sign in to view

Copy link
@nlf

nlf Aug 12, 2021

Author Contributor

this makes it deterministic and does skip looking at the directory name for the root node.. the previous behavior got us into a situation like the following:

> mkdir foo
> cd foo
> npm init -y # package.json will have `name: "foo"`
> npm i # in package-lock.json, the "" entry in packages will have no name field
> cd ../
> mkdir foo2
> cd foo2
> cp ../foo/package.json . # same package.json, name = "foo"
> npm i # in package-lock.json, the "" entry in packages will have `name: "foo"`

this change makes it so the "" entry in packages will always have a name field, and always set to the package name, rather than removing it if the directory name matches the package name, and adding it back if they do not match

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Aug 12, 2021

Contributor

gotcha, thanks!
// project is in
const pname = node.packageName
if (pname && pname !== node.name)
if (pname && (node === node.root || pname !== node.name))
meta.name = pname

if (node.isTop && node.package.devDependencies)
Expand Down
19 changes: 19 additions & 0 deletions tap-snapshots/test/shrinkwrap.js.test.cjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ Object {
"e": "https://foo.com/e.tgz",
},
"hasInstallScript": true,
"name": "root",
"optionalDependencies": Object {
"optin": "",
},
Expand Down Expand Up @@ -641,6 +642,7 @@ Object {
"e": "https://foo.com/e.tgz",
},
"hasInstallScript": true,
"name": "root",
"optionalDependencies": Object {
"optin": "",
},
Expand All @@ -663,6 +665,7 @@ Object {
"e": "https://foo.com/e.tgz",
},
"hasInstallScript": true,
"name": "root",
"optionalDependencies": Object {
"optin": "",
},
Expand Down Expand Up @@ -1236,6 +1239,7 @@ Object {
"dependencies": Object {
"dep": "",
},
"name": "bundle",
},
"node_modules/dep": Object {
"inBundle": true,
Expand Down Expand Up @@ -1583,6 +1587,7 @@ Object {
"devDependencies": Object {
"a": "",
},
"name": "devloop",
},
"node_modules/a": Object {
"dependencies": Object {
Expand Down Expand Up @@ -1621,6 +1626,7 @@ Object {
"dependencies": Object {
"dep": "",
},
"name": "root",
"version": "1.0.0",
},
},
Expand Down Expand Up @@ -1653,6 +1659,7 @@ Object {
"x": "",
"z": "",
},
"name": "root",
},
"../a": Object {},
"../a/node_modules/b": Object {
Expand Down Expand Up @@ -1735,6 +1742,7 @@ Object {
"o2": "file:../m/node_modules/n/o2",
"x": "",
},
"name": "root",
"version": "1.0.0",
},
"../a": Object {},
Expand Down Expand Up @@ -2420,6 +2428,7 @@ Object {
"dependencies": Object {
"bork": "file:..",
},
"name": "root",
"version": "1.2.3",
},
"..": Object {
Expand Down Expand Up @@ -2721,6 +2730,7 @@ Object {
"link-outside-nest": "",
"nest": "",
},
"name": "links-all-over",
"version": "1.2.3",
},
"node_modules/link-deep": Object {
Expand Down Expand Up @@ -3089,6 +3099,7 @@ Object {
"dependencies": Object {
"c": "",
},
"name": "optionalloop",
"optionalDependencies": Object {
"a": "",
},
Expand Down Expand Up @@ -3162,6 +3173,7 @@ Object {
"devDependencies": Object {
"a": "",
},
"name": "optofdev",
},
"node_modules/a": Object {
"dependencies": Object {
Expand Down Expand Up @@ -3478,6 +3490,7 @@ Object {
"devDependencies": Object {
"foo": "*",
},
"name": "root",
"optionalDependencies": Object {
"notinstalledhere": "",
},
Expand Down Expand Up @@ -3657,6 +3670,7 @@ Object {
"@scope/y": "",
"foo": "",
},
"name": "selflink",
"version": "1.2.3",
},
"node_modules/@scope/y": Object {
Expand Down Expand Up @@ -3736,6 +3750,7 @@ Object {
"name": "example",
"packages": Object {
"": Object {
"name": "example",
"version": "1.0.0",
},
"../bar": Object {
Expand Down Expand Up @@ -13762,6 +13777,7 @@ Object {
"b": "",
"c": "",
},
"name": "workspace",
},
"node_modules/a": Object {
"link": true,
Expand Down Expand Up @@ -13979,6 +13995,9 @@ Object {
"lockfileVersion": 2,
"name": "workspace3",
"packages": Object {
"": Object {
"name": "workspace3",
},
"app": Object {
"dependencies": Object {
"a": "",
Expand Down

0 comments on commit 32cdebb

Please sign in to comment.