[npm#5280] Ensure that flags like --save-dev imply --save, and are no…

…t overridden by npm config
ficocelliguy · Mar 1, 2024 · 1f3c284 · 1f3c284
1 parent 0b5416c
commit 1f3c284
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 33 deletions.
diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js
@@ -1813,7 +1813,6 @@ define('save-dev', {
       return
     }
 
-    flatOptions.save = true
     flatOptions.saveType = 'dev'
   },
 })
@@ -1850,8 +1849,6 @@ define('save-optional', {
       return
     }
 
-    flatOptions.save = true
-
     if (flatOptions.saveType === 'peerOptional') {
       return
     }
@@ -1880,8 +1877,6 @@ define('save-peer', {
       return
     }
 
-    flatOptions.save = true
-
     if (flatOptions.saveType === 'peerOptional') {
       return
     }
@@ -1933,7 +1928,6 @@ define('save-prod', {
       return
     }
 
-    flatOptions.save = true
     flatOptions.saveType = 'prod'
   },
 })

diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js
@@ -232,6 +232,11 @@ class Config {
     for (const { data } of this.data.values()) {
       this.#flatten(data, this.#flatOptions)
     }
+
+    // Ensure 'save' is true if a saveType has been specified
+    if (this.#flatOptions.saveType) {
+      this.#flatOptions.save = true
+    }
     this.#flatOptions.nodeBin = this.execPath
     this.#flatOptions.npmBin = this.npmBin
     process.emit('timeEnd', 'config:load:flatten')

diff --git a/workspaces/config/test/definitions/definitions.js b/workspaces/config/test/definitions/definitions.js
@@ -583,8 +583,7 @@ t.test('saveType', t => {
     t.strictSame(flat, { saveType: 'dev' }, 'ignore if false and not already prod')
     obj['save-prod'] = true
     mockDefs()['save-prod'].flatten('save-prod', obj, flat)
-    t.strictSame(flat,
-      { saveType: 'prod', save: true }, 'set to prod if true, and set save to true')
+    t.strictSame(flat, { saveType: 'prod' }, 'set to prod if true')
     t.end()
   })
 
@@ -602,7 +601,7 @@ t.test('saveType', t => {
     t.strictSame(flat, { saveType: 'prod' }, 'ignore if false and not already dev')
     obj['save-dev'] = true
     mockDefs()['save-dev'].flatten('save-dev', obj, flat)
-    t.strictSame(flat, { saveType: 'dev', save: true }, 'set to dev if true, and set save to true')
+    t.strictSame(flat, { saveType: 'dev' }, 'set to dev if true')
     t.end()
   })
 
@@ -626,33 +625,27 @@ t.test('saveType', t => {
 
   t.test('save-peer', t => {
     const obj = { 'save-peer': false }
-    let flat = {}
+    const flat = {}
     mockDefs()['save-peer'].flatten('save-peer', obj, flat)
     t.strictSame(flat, {}, 'no effect if false and not yet set')
 
     obj['save-peer'] = true
     mockDefs()['save-peer'].flatten('save-peer', obj, flat)
-    t.strictSame(flat,
-      { saveType: 'peer', save: true }, 'set saveType to peer if unset, and set save to true')
+    t.strictSame(flat, { saveType: 'peer' }, 'set saveType to peer if unset')
 
-    flat = { saveType: 'optional' }
+    flat.saveType = 'optional'
     mockDefs()['save-peer'].flatten('save-peer', obj, flat)
-    t.strictSame(flat,
-      { saveType: 'peerOptional', save: true },
-      'set to peerOptional, and set save to true if optional already')
+    t.strictSame(flat, { saveType: 'peerOptional' }, 'set to peerOptional if optional already')
 
-    flat = { saveType: 'optional' }
     mockDefs()['save-peer'].flatten('save-peer', obj, flat)
-    t.strictSame(flat,
-      { saveType: 'peerOptional', save: true }, 'just set save to true if already peerOptional')
+    t.strictSame(flat, { saveType: 'peerOptional' }, 'no effect if already peerOptional')
 
     obj['save-peer'] = false
-    flat = { saveType: 'optional' }
     mockDefs()['save-peer'].flatten('save-peer', obj, flat)
     t.strictSame(flat, { saveType: 'optional' }, 'switch peerOptional to optional if false')
 
     obj['save-peer'] = false
-    flat = { saveType: 'peer' }
+    flat.saveType = 'peer'
     mockDefs()['save-peer'].flatten('save-peer', obj, flat)
     t.strictSame(flat, {}, 'remove saveType if peer and setting false')
 
@@ -661,32 +654,26 @@ t.test('saveType', t => {
 
   t.test('save-optional', t => {
     const obj = { 'save-optional': false }
-    let flat = {}
+    const flat = {}
     mockDefs()['save-optional'].flatten('save-optional', obj, flat)
     t.strictSame(flat, {}, 'no effect if false and not yet set')
 
     obj['save-optional'] = true
     mockDefs()['save-optional'].flatten('save-optional', obj, flat)
-    t.strictSame(flat, { saveType: 'optional', save: true },
-      'set saveType to optional if unset, and set save to true')
+    t.strictSame(flat, { saveType: 'optional' }, 'set saveType to optional if unset')
 
-    flat = { saveType: 'peer' }
+    flat.saveType = 'peer'
     mockDefs()['save-optional'].flatten('save-optional', obj, flat)
-    t.strictSame(flat,
-      { saveType: 'peerOptional', save: true },
-      'set to peerOptional if peer already, and set save to true')
+    t.strictSame(flat, { saveType: 'peerOptional' }, 'set to peerOptional if peer already')
 
-    flat = { saveType: 'peer' }
     mockDefs()['save-optional'].flatten('save-optional', obj, flat)
-    t.strictSame(flat,
-      { saveType: 'peerOptional', save: true }, 'just set save to true if already peerOptional')
+    t.strictSame(flat, { saveType: 'peerOptional' }, 'no effect if already peerOptional')
 
     obj['save-optional'] = false
-    flat = { saveType: 'peer' }
     mockDefs()['save-optional'].flatten('save-optional', obj, flat)
     t.strictSame(flat, { saveType: 'peer' }, 'switch peerOptional to peer if false')
 
-    flat = { saveType: 'optional' }
+    flat.saveType = 'optional'
     mockDefs()['save-optional'].flatten('save-optional', obj, flat)
     t.strictSame(flat, {}, 'remove saveType if optional and setting false')
 

diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js
@@ -908,6 +908,55 @@ t.test('finding the global prefix', t => {
   t.end()
 })
 
+t.test('manages the save flag when flat is retrieved', t => {
+  const npmPath = __dirname
+  t.test('does not set save to true if a save flag is not passed', async t => {
+    const c = new Config({
+      argv: [process.execPath, __filename],
+      shorthands,
+      definitions,
+      npmPath,
+      flatten,
+    })
+    await c.load()
+    t.equal(c.flat.save, false)
+  })
+  t.test('does not set save to true if flag is passed that does not efffect saveType', async t => {
+    const c = new Config({
+      argv: [process.execPath, __filename, '--save-exact'],
+      shorthands,
+      definitions,
+      npmPath,
+      flatten,
+    })
+    await c.load()
+    t.equal(c.flat.save, false)
+  })
+  t.test('does not set save to true if a negative save flag is passed', async t => {
+    const c = new Config({
+      argv: [process.execPath, __filename, '--save-dev=false'],
+      shorthands,
+      definitions,
+      npmPath,
+      flatten,
+    })
+    await c.load()
+    t.equal(c.flat.save, false)
+  })
+  t.test('sets save to true if a save flag is passed', async t => {
+    const c = new Config({
+      argv: [process.execPath, __filename, '--save-prod'],
+      shorthands,
+      definitions,
+      npmPath,
+      flatten,
+    })
+    await c.load()
+    t.equal(c.flat.save, true)
+  })
+  t.end()
+})
+
 t.test('finding the local prefix', t => {
   const path = t.testdir({
     hasNM: {