fix: set lockfileVersion from file during reset

Fix: npm/cli#3920

When loading the initial tree while updating all, a shrinkwrap file
loaded from disk with an existing `lockfileVersion` was not having the
`lockfileVersion` preserved when saving the file back to disk.

With this patch, an existing `lockfileVersion` is preserved if it is
greater than the `defaultLockfileVersion`.

Co-authored-by: @isaacs

PR-URL: #340
Credit: @lukekarrys
Close: #340
Reviewed-by: @isaacs

lib/shrinkwrap.js

@@ -238,21 +238,31 @@ class Shrinkwrap {
     return swKeyOrder
   }
 
-  static reset (options) {
+  static async reset (options) {
     // still need to know if it was loaded from the disk, but don't
     // bother reading it if we're gonna just throw it away.
     const s = new Shrinkwrap(options)
     s.reset()
 
-    return s[_maybeStat]().then(([sw, lock]) => {
-      s.filename = resolve(s.path,
-        (s.hiddenLockfile ? 'node_modules/.package-lock'
-        : s.shrinkwrapOnly || sw ? 'npm-shrinkwrap'
-        : 'package-lock') + '.json')
-      s.loadedFromDisk = !!(sw || lock)
-      s.type = basename(s.filename)
-      return s
-    })
+    const [sw, lock] = await s[_maybeStat]()
+
+    s.filename = resolve(s.path,
+      (s.hiddenLockfile ? 'node_modules/.package-lock'
+      : s.shrinkwrapOnly || sw ? 'npm-shrinkwrap'
+      : 'package-lock') + '.json')
+    s.loadedFromDisk = !!(sw || lock)
+    s.type = basename(s.filename)
+
+    try {
+      if (s.loadedFromDisk && !s.lockfileVersion) {
+        const json = parseJSON(await maybeReadFile(s.filename))
+        if (json.lockfileVersion > defaultLockfileVersion) {
+          s.lockfileVersion = json.lockfileVersion
+        }
+      }
+    } catch (e) {}
+
+    return s
   }
 
   static metaFromNode (node, path) {

test/arborist/build-ideal-tree.js

@@ -638,6 +638,34 @@ t.test('empty update should not trigger old lockfile', async t => {
   t.strictSame(checkLogs(), [])
 })
 
+t.test('update v3 doesnt downgrade lockfile', async t => {
+  const fixt = t.testdir({
+    'package-lock.json': JSON.stringify({
+      name: 'empty-update-v3',
+      version: '1.0.0',
+      lockfileVersion: 3,
+      requires: true,
+      packages: {
+        '': {
+          name: 'empty-update-v3',
+          version: '1.0.0',
+        },
+      },
+    }),
+    'package.json': JSON.stringify({
+      name: 'empty-update-v3',
+      version: '1.0.0',
+    }),
+  })
+
+  const arb = newArb(fixt)
+  await arb.reify({ update: true })
+
+  const { lockfileVersion } = require(resolve(fixt, 'package-lock.json'))
+
+  t.strictSame(lockfileVersion, 3)
+})
+
 t.test('no fix available', async t => {
   const path = resolve(fixtures, 'audit-mkdirp/mkdirp-unfixable')
   const checkLogs = warningTracker()

test/shrinkwrap.js

@@ -80,6 +80,35 @@ t.test('starting out with a reset lockfile is an empty lockfile', t =>
     t.equal(sw.filename, resolve(fixture, 'package-lock.json'))
   }))
 
+t.test('reset in a bad dir gets an empty lockfile with no lockfile version', async t => {
+  const nullLockDir = t.testdir({
+    'package-lock.json': JSON.stringify(null),
+  })
+
+  const [swMissingLock, swNullLock] = await Promise.all([
+    Shrinkwrap.reset({ path: 'path/which/does/not/exist' }),
+    Shrinkwrap.reset({ path: nullLockDir }),
+  ])
+
+  t.strictSame(swMissingLock.data, {
+    lockfileVersion: 2,
+    requires: true,
+    dependencies: {},
+    packages: {},
+  })
+  t.equal(swMissingLock.lockfileVersion, null)
+  t.equal(swMissingLock.loadedFromDisk, false)
+
+  t.strictSame(swNullLock.data, {
+    lockfileVersion: 2,
+    requires: true,
+    dependencies: {},
+    packages: {},
+  })
+  t.equal(swNullLock.lockfileVersion, null)
+  t.equal(swNullLock.loadedFromDisk, true)
+})
+
 t.test('loading in bad dir gets empty lockfile', t =>
   Shrinkwrap.load({ path: 'path/which/does/not/exist' }).then(sw => {
     t.strictSame(sw.data, {
@@ -1509,37 +1538,43 @@ t.test('setting lockfileVersion from the file contents', async t => {
       'package-lock.json': JSON.stringify({ lockfileVersion: 3 }),
     },
   })
+
+  const loadAndReset = (options) => Promise.all([
+    Shrinkwrap.load(options).then((s) => s.lockfileVersion),
+    Shrinkwrap.reset(options).then((s) => s.lockfileVersion),
+  ])
+
   t.test('default setting', async t => {
-    const s1 = await Shrinkwrap.load({ path: `${path}/v1` })
-    t.equal(s1.lockfileVersion, 2, 'will upgrade old lockfile')
-    const s2 = await Shrinkwrap.load({ path: `${path}/v2` })
-    t.equal(s2.lockfileVersion, 2, 'will keep v2 as v2')
-    const s3 = await Shrinkwrap.load({ path: `${path}/v3` })
-    t.equal(s3.lockfileVersion, 3, 'will keep v3 as v3')
+    const s1 = await loadAndReset({ path: `${path}/v1` })
+    t.strictSame(s1, [2, null], 'will upgrade old lockfile')
+    const s2 = await loadAndReset({ path: `${path}/v2` })
+    t.strictSame(s2, [2, null], 'will keep v2 as v2')
+    const s3 = await loadAndReset({ path: `${path}/v3` })
+    t.strictSame(s3, [3, 3], 'load will keep v3 as v3')
   })
   t.test('v1', async t => {
-    const s1 = await Shrinkwrap.load({ path: `${path}/v1`, lockfileVersion: 1 })
-    t.equal(s1.lockfileVersion, 1, 'keep explicit v1 setting')
-    const s2 = await Shrinkwrap.load({ path: `${path}/v2`, lockfileVersion: 1 })
-    t.equal(s2.lockfileVersion, 1, 'downgrade explicitly')
-    const s3 = await Shrinkwrap.load({ path: `${path}/v3`, lockfileVersion: 1 })
-    t.equal(s3.lockfileVersion, 1, 'downgrade explicitly')
+    const s1 = await loadAndReset({ path: `${path}/v1`, lockfileVersion: 1 })
+    t.strictSame(s1, [1, 1], 'keep explicit v1 setting')
+    const s2 = await loadAndReset({ path: `${path}/v2`, lockfileVersion: 1 })
+    t.strictSame(s2, [1, 1], 'downgrade explicitly')
+    const s3 = await loadAndReset({ path: `${path}/v3`, lockfileVersion: 1 })
+    t.strictSame(s3, [1, 1], 'downgrade explicitly')
   })
   t.test('v2', async t => {
-    const s1 = await Shrinkwrap.load({ path: `${path}/v1`, lockfileVersion: 2 })
-    t.equal(s1.lockfileVersion, 2, 'upgrade explicitly')
-    const s2 = await Shrinkwrap.load({ path: `${path}/v2`, lockfileVersion: 2 })
-    t.equal(s2.lockfileVersion, 2, 'keep v2 setting')
-    const s3 = await Shrinkwrap.load({ path: `${path}/v3`, lockfileVersion: 2 })
-    t.equal(s3.lockfileVersion, 2, 'downgrade explicitly')
+    const s1 = await loadAndReset({ path: `${path}/v1`, lockfileVersion: 2 })
+    t.strictSame(s1, [2, 2], 'upgrade explicitly')
+    const s2 = await loadAndReset({ path: `${path}/v2`, lockfileVersion: 2 })
+    t.strictSame(s2, [2, 2], 'keep v2 setting')
+    const s3 = await loadAndReset({ path: `${path}/v3`, lockfileVersion: 2 })
+    t.strictSame(s3, [2, 2], 'downgrade explicitly')
   })
   t.test('v3', async t => {
-    const s1 = await Shrinkwrap.load({ path: `${path}/v1`, lockfileVersion: 3 })
-    t.equal(s1.lockfileVersion, 3, 'upgrade explicitly')
-    const s2 = await Shrinkwrap.load({ path: `${path}/v2`, lockfileVersion: 3 })
-    t.equal(s2.lockfileVersion, 3, 'upgrade explicitly')
-    const s3 = await Shrinkwrap.load({ path: `${path}/v3`, lockfileVersion: 3 })
-    t.equal(s3.lockfileVersion, 3, 'keep v3 setting')
+    const s1 = await loadAndReset({ path: `${path}/v1`, lockfileVersion: 3 })
+    t.strictSame(s1, [3, 3], 'upgrade explicitly')
+    const s2 = await loadAndReset({ path: `${path}/v2`, lockfileVersion: 3 })
+    t.strictSame(s2, [3, 3], 'upgrade explicitly')
+    const s3 = await loadAndReset({ path: `${path}/v3`, lockfileVersion: 3 })
+    t.strictSame(s3, [3, 3], 'keep v3 setting')
   })
 
   t.equal(Shrinkwrap.defaultLockfileVersion, 2, 'default is 2')