feat: Do not implicitly downgrade lockfileVersion from 3 to 2

If lockfileVersion is explicitly set to 3, and then later it is not
explicitly set, then we continue to use a version 3 lockfile.

If the lockfileVersion is explicitly set, we always use the version in
the options.

Also adds validation to the lockfileVersion config option, and cleans up
the logic in old/ancient lockfile inflation, so that we're not doing
that extra work if there just isn't a lockfile at all.

Finally, this also corrects a weird edge case where we were doing the
"inflate old lockfile" behavior when there is no lockfile present and we
load the initial idealTree state from the actualTree.  That would result
in adding integrity values for packages found present in the tree.
However, as we cannot be sure that `node_modules/foo` came from
`{registry}/foo`, this is an assertion we can't guarantee.  No one has
yet reported this obscure edge case as causing problems.

PR-URL: #329
Credit: @isaacs
Close: #329
Reviewed-by: @ljharb, @wraithgar

Author: isaacs

lib/arborist/build-ideal-tree.js

@@ -26,6 +26,7 @@ const debug = require('../debug.js')
 const fromPath = require('../from-path.js')
 const calcDepFlags = require('../calc-dep-flags.js')
 const Shrinkwrap = require('../shrinkwrap.js')
+const { defaultLockfileVersion } = Shrinkwrap
 const Node = require('../node.js')
 const Link = require('../link.js')
 const addRmPkgDeps = require('../add-rm-pkg-deps.js')
@@ -328,6 +329,9 @@ module.exports = cls => class IdealTreeBuilder extends cls {
           // dep flags before assuming that any mutations were reflected.
           if (tree.children.size) {
             root.meta.loadedFromDisk = true
+            // set these so that we don't try to ancient lockfile reload it
+            root.meta.originalLockfileVersion = defaultLockfileVersion
+            root.meta.lockfileVersion = defaultLockfileVersion
           }
         }
         return root
@@ -701,14 +705,18 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     // least it's just a one-time hit.
     process.emit('time', 'idealTree:inflate')
 
+    // don't warn if we're not gonna actually write it back anyway.
     const heading = ancient ? 'ancient lockfile' : 'old lockfile'
-    this.log.warn(heading,
-      `
+    if (ancient || !this.options.lockfileVersion ||
+        this.options.lockfileVersion >= defaultLockfileVersion) {
+      this.log.warn(heading,
+        `
 The ${meta.type} file was created with an old version of npm,
 so supplemental metadata must be fetched from the registry.
 
 This is a one-time fix-up, please be patient...
 `)
+    }
 
     this.addTracker('idealTree:inflate')
     const queue = []
@@ -753,7 +761,7 @@ This is a one-time fix-up, please be patient...
     // yes, yes, this isn't the "original" version, but now that it's been
     // upgraded, we need to make sure we don't do the work to upgrade it
     // again, since it's now as new as can be.
-    meta.originalLockfileVersion = 2
+    meta.originalLockfileVersion = defaultLockfileVersion
     this.finishTracker('idealTree:inflate')
     process.emit('timeEnd', 'idealTree:inflate')
   }

lib/arborist/index.js

@@ -48,6 +48,21 @@ const _workspacesEnabled = Symbol.for('workspacesEnabled')
 const Base = mixins.reduce((a, b) => b(a), require('events'))
 const getWorkspaceNodes = require('../get-workspace-nodes.js')
 
+// if it's 1, 2, or 3, set it explicitly that.
+// if undefined or null, set it null
+// otherwise, throw.
+const lockfileVersion = lfv => {
+  if (lfv === 1 || lfv === 2 || lfv === 3) {
+    return lfv
+  }
+
+  if (lfv === undefined || lfv === null) {
+    return null
+  }
+
+  throw new TypeError('Invalid lockfileVersion config: ' + lfv)
+}
+
 class Arborist extends Base {
   constructor (options = {}) {
     process.emit('time', 'arborist:ctor')
@@ -60,6 +75,7 @@ class Arborist extends Base {
       packumentCache: options.packumentCache || new Map(),
       log: options.log || procLog,
       workspacesEnabled: options.workspacesEnabled !== false,
+      lockfileVersion: lockfileVersion(options.lockfileVersion),
     }
 
     this[_workspacesEnabled] = this.options.workspacesEnabled

lib/arborist/load-actual.js

@@ -137,15 +137,12 @@ module.exports = cls => class ActualLoader extends cls {
       real: await realpath(this.path, this[_rpcache], this[_stcache]),
     })
 
-    // XXX only rely on this if the hidden lockfile is the newest thing?
-    // need some kind of heuristic, like if the package.json or sw have
-    // been touched more recently, then ignore it?  This is a hazard if
-    // user switches back and forth between Arborist and another way of
-    // mutating the node_modules folder.
+    // Note: hidden lockfile will be rejected if it's not the latest thing
+    // in the folder, or if any of the entries in the hidden lockfile are
+    // missing.
     const meta = await Shrinkwrap.load({
       path: this[_actualTree].path,
       hiddenLockfile: true,
-      lockfileVersion: 3,
     })
     if (meta.loadedFromDisk) {
       this[_actualTree].meta = meta

lib/shrinkwrap.js

@@ -226,6 +226,10 @@ const _filenameSet = Symbol('_filenameSet')
 const _maybeRead = Symbol('_maybeRead')
 const _maybeStat = Symbol('_maybeStat')
 class Shrinkwrap {
+  static get defaultLockfileVersion () {
+    return defaultLockfileVersion
+  }
+
   static load (options) {
     return new Shrinkwrap(options).load()
   }
@@ -316,10 +320,12 @@ class Shrinkwrap {
       shrinkwrapOnly = false,
       hiddenLockfile = false,
       log = procLog,
-      lockfileVersion = defaultLockfileVersion,
+      lockfileVersion,
     } = options
 
-    this.lockfileVersion = parseInt(hiddenLockfile ? 3 : lockfileVersion, 10)
+    this.lockfileVersion = hiddenLockfile ? 3
+      : lockfileVersion ? parseInt(lockfileVersion, 10)
+      : null
     this.log = log
     this[_awaitingUpdate] = new Map()
     this.tree = null
@@ -376,7 +382,7 @@ class Shrinkwrap {
     this[_awaitingUpdate] = new Map()
     this.originalLockfileVersion = this.lockfileVersion
     this.data = {
-      lockfileVersion: this.lockfileVersion,
+      lockfileVersion: this.lockfileVersion || defaultLockfileVersion,
       requires: true,
       packages: {},
       dependencies: {},
@@ -462,15 +468,23 @@ class Shrinkwrap {
       this.ancientLockfile = false
       return {}
     }).then(lock => {
+      const lockfileVersion = this.lockfileVersion ? this.lockfileVersion
+        : Math.max(lock.lockfileVersion || 0, defaultLockfileVersion)
       this.data = {
         ...lock,
-        lockfileVersion: this.lockfileVersion,
+        lockfileVersion: lockfileVersion,
         requires: true,
         packages: lock.packages || {},
         dependencies: lock.dependencies || {},
       }
 
       this.originalLockfileVersion = lock.lockfileVersion
+      // use default if it wasn't explicitly set, and the current file is
+      // less than our default.  otherwise, keep whatever is in the file,
+      // unless we had an explicit setting already.
+      if (!this.lockfileVersion) {
+        this.lockfileVersion = this.data.lockfileVersion = lockfileVersion
+      }
       this.ancientLockfile = this.loadedFromDisk &&
         !(lock.lockfileVersion >= 2) && !lock.requires
 
@@ -881,25 +895,32 @@ class Shrinkwrap {
       }
     }
 
+    // if we haven't set it by now, use the default
+    if (!this.lockfileVersion) {
+      this.lockfileVersion = defaultLockfileVersion
+    }
+    this.data.lockfileVersion = this.lockfileVersion
+
     // hidden lockfiles don't include legacy metadata or a root entry
     if (this.hiddenLockfile) {
       delete this.data.packages['']
       delete this.data.dependencies
-    } else if (this.tree) {
+    } else if (this.tree && this.lockfileVersion <= 3) {
       this[_buildLegacyLockfile](this.tree, this.data)
     }
 
-    const data = { ...this.data }
     // lf version 1 = dependencies only
     // lf version 2 = dependencies and packages
     // lf version 3 = packages only
     if (this.lockfileVersion >= 3) {
-      delete data.dependencies
+      const { dependencies, ...data } = this.data
+      return data
     } else if (this.lockfileVersion < 2) {
-      delete data.packages
+      const { packages, ...data } = this.data
+      return data
+    } else {
+      return { ...this.data }
     }
-
-    return data
   }
 
   [_buildLegacyLockfile] (node, lock, path = []) {