From 0b0758700e78cc9bc47df369709f7f5c8f81a135 Mon Sep 17 00:00:00 2001 From: Michael Ficocelli Date: Mon, 19 Feb 2024 17:31:48 -0500 Subject: [PATCH 1/6] [npm#5280] Ensure that flags like --save-dev imply --save, and are not overridden by npm config --- .../config/lib/definitions/definitions.js | 9 ++++ .../config/test/definitions/definitions.js | 46 ++++++++++++------- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index 6f8760fce1d3e..56f642b2c69e1 100644 --- a/workspaces/config/lib/definitions/definitions.js +++ b/workspaces/config/lib/definitions/definitions.js @@ -1813,6 +1813,7 @@ define('save-dev', { return } + flatOptions.save = true flatOptions.saveType = 'dev' }, }) @@ -1826,6 +1827,9 @@ define('save-exact', { version rather than using npm's default semver range operator. `, flatten (key, obj, flatOptions) { + if (obj[key]) { + flatOptions.save = true + } // just call the save-prefix flattener, it reads from obj['save-exact'] definitions['save-prefix'].flatten('save-prefix', obj, flatOptions) }, @@ -1849,6 +1853,8 @@ define('save-optional', { return } + flatOptions.save = true + if (flatOptions.saveType === 'peerOptional') { return } @@ -1877,6 +1883,8 @@ define('save-peer', { return } + flatOptions.save = true + if (flatOptions.saveType === 'peerOptional') { return } @@ -1928,6 +1936,7 @@ define('save-prod', { return } + flatOptions.save = true flatOptions.saveType = 'prod' }, }) diff --git a/workspaces/config/test/definitions/definitions.js b/workspaces/config/test/definitions/definitions.js index a832f7da0b6cb..94e118b475d5b 100644 --- a/workspaces/config/test/definitions/definitions.js +++ b/workspaces/config/test/definitions/definitions.js @@ -583,7 +583,8 @@ 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' }, 'set to prod if true') + t.strictSame(flat, + { saveType: 'prod', save: true }, 'set to prod if true, and set save to true') t.end() }) @@ -601,7 +602,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' }, 'set to dev if true') + t.strictSame(flat, { saveType: 'dev', save: true }, 'set to dev if true, and set save to true') t.end() }) @@ -625,27 +626,33 @@ t.test('saveType', t => { t.test('save-peer', t => { const obj = { 'save-peer': false } - const flat = {} + let 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' }, 'set saveType to peer if unset') + t.strictSame(flat, + { saveType: 'peer', save: true }, 'set saveType to peer if unset, and set save to true') - flat.saveType = 'optional' + flat = { saveType: 'optional' } mockDefs()['save-peer'].flatten('save-peer', obj, flat) - t.strictSame(flat, { saveType: 'peerOptional' }, 'set to peerOptional if optional already') + t.strictSame(flat, + { saveType: 'peerOptional', save: true }, + 'set to peerOptional, and set save to true if optional already') + flat = { saveType: 'optional' } mockDefs()['save-peer'].flatten('save-peer', obj, flat) - t.strictSame(flat, { saveType: 'peerOptional' }, 'no effect if already peerOptional') + t.strictSame(flat, + { saveType: 'peerOptional', save: true }, 'just set save to true 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') @@ -654,26 +661,32 @@ t.test('saveType', t => { t.test('save-optional', t => { const obj = { 'save-optional': false } - const flat = {} + let 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' }, 'set saveType to optional if unset') + t.strictSame(flat, { saveType: 'optional', save: true }, + 'set saveType to optional if unset, and set save to true') - flat.saveType = 'peer' + flat = { saveType: 'peer' } mockDefs()['save-optional'].flatten('save-optional', obj, flat) - t.strictSame(flat, { saveType: 'peerOptional' }, 'set to peerOptional if peer already') + t.strictSame(flat, + { saveType: 'peerOptional', save: true }, + 'set to peerOptional if peer already, and set save to true') + flat = { saveType: 'peer' } mockDefs()['save-optional'].flatten('save-optional', obj, flat) - t.strictSame(flat, { saveType: 'peerOptional' }, 'no effect if already peerOptional') + t.strictSame(flat, + { saveType: 'peerOptional', save: true }, 'just set save to true 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') @@ -801,10 +814,11 @@ t.test('save-exact', t => { 'save-exact': true, 'save-prefix': '~1.2.3', } - const flat = {} + let flat = {} mockDefs()['save-exact'] .flatten('save-exact', { ...obj, 'save-exact': true }, flat) - t.strictSame(flat, { savePrefix: '' }) + t.strictSame(flat, { savePrefix: '', save: true }) + flat = {} mockDefs()['save-exact'] .flatten('save-exact', { ...obj, 'save-exact': false }, flat) t.strictSame(flat, { savePrefix: '~1.2.3' }) From 0b5416cdf75af9bead5b64961af79d1e140110f8 Mon Sep 17 00:00:00 2001 From: Michael Ficocelli Date: Tue, 20 Feb 2024 14:25:26 -0500 Subject: [PATCH 2/6] [npm#5280] Review feedback: do not force save on save-exact --- workspaces/config/lib/definitions/definitions.js | 3 --- workspaces/config/test/definitions/definitions.js | 5 ++--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index 56f642b2c69e1..a433bf4f9bed2 100644 --- a/workspaces/config/lib/definitions/definitions.js +++ b/workspaces/config/lib/definitions/definitions.js @@ -1827,9 +1827,6 @@ define('save-exact', { version rather than using npm's default semver range operator. `, flatten (key, obj, flatOptions) { - if (obj[key]) { - flatOptions.save = true - } // just call the save-prefix flattener, it reads from obj['save-exact'] definitions['save-prefix'].flatten('save-prefix', obj, flatOptions) }, diff --git a/workspaces/config/test/definitions/definitions.js b/workspaces/config/test/definitions/definitions.js index 94e118b475d5b..7de490f044bd3 100644 --- a/workspaces/config/test/definitions/definitions.js +++ b/workspaces/config/test/definitions/definitions.js @@ -814,11 +814,10 @@ t.test('save-exact', t => { 'save-exact': true, 'save-prefix': '~1.2.3', } - let flat = {} + const flat = {} mockDefs()['save-exact'] .flatten('save-exact', { ...obj, 'save-exact': true }, flat) - t.strictSame(flat, { savePrefix: '', save: true }) - flat = {} + t.strictSame(flat, { savePrefix: '' }) mockDefs()['save-exact'] .flatten('save-exact', { ...obj, 'save-exact': false }, flat) t.strictSame(flat, { savePrefix: '~1.2.3' }) From 1f3c2847b1ec323fe3acb4a09993901c31718b79 Mon Sep 17 00:00:00 2001 From: Michael Ficocelli Date: Fri, 1 Mar 2024 16:55:46 -0500 Subject: [PATCH 3/6] [npm#5280] Ensure that flags like --save-dev imply --save, and are not overridden by npm config --- .../config/lib/definitions/definitions.js | 6 --- workspaces/config/lib/index.js | 5 ++ .../config/test/definitions/definitions.js | 41 ++++++---------- workspaces/config/test/index.js | 49 +++++++++++++++++++ 4 files changed, 68 insertions(+), 33 deletions(-) diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index a433bf4f9bed2..6f8760fce1d3e 100644 --- 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 index b09ecc478f64f..20efdcf38a1d3 100644 --- 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 index 7de490f044bd3..a832f7da0b6cb 100644 --- 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 index 9e9fac4f6dc34..1551f2bc523c5 100644 --- 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: { From f7e4d7d21f85d214278b17dfb082eedcd54dda3d Mon Sep 17 00:00:00 2001 From: Michael Ficocelli Date: Tue, 5 Mar 2024 11:09:52 -0500 Subject: [PATCH 4/6] [npm#5280] Detect default save, improve tests --- workspaces/config/lib/index.js | 5 ++-- workspaces/config/test/index.js | 52 +++++++++++++++------------------ 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index 20efdcf38a1d3..158bef8b8c8bf 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -233,8 +233,9 @@ class Config { this.#flatten(data, this.#flatOptions) } - // Ensure 'save' is true if a saveType has been specified - if (this.#flatOptions.saveType) { + // Only set 'save' to true if a saveType has been specified + // and if 'save' is false due to non-default config + if (!this.isDefault('save') && this.#flatOptions.saveType) { this.#flatOptions.save = true } this.#flatOptions.nodeBin = this.execPath diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index 1551f2bc523c5..88b238967e610 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -910,48 +910,42 @@ t.test('finding the global prefix', t => { 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 buildConfig = async (args = [], envSave = false) => { const c = new Config({ - argv: [process.execPath, __filename], + argv: [process.execPath, __filename, ...args], shorthands, definitions, npmPath, flatten, + env: { + save: envSave, + }, }) await c.load() + // Ensure test runner environment's npm settings do not change test outcomes + c.set('save', envSave, 'user') + c.set('save', envSave, 'global') + c.set('save', envSave, 'project') + return c + } + t.test('does not override save to true if a save flag is not passed', async t => { + const c = await buildConfig([], false) 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.test('does not override save to true if a negative save flag is passed', async t => { + const c = await buildConfig(['--save-dev=false'], false) 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.test('overrides save to true if a save flag is passed', async t => { + const c = await buildConfig(['--save-prod'], false) + t.equal(c.flat.save, true) + }) + t.test('does not overwrite save if --no-save is present', async t => { + const c = await buildConfig(['--no-save'], true) 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.test('overwrites save if --no-save is present and also a save flag', async t => { + const c = await buildConfig(['--save-prod', '--no-save'], false) t.equal(c.flat.save, true) }) t.end() From 7db7c477ed84a2b325c95c59c23878c9af7f7ac9 Mon Sep 17 00:00:00 2001 From: Michael Ficocelli Date: Tue, 5 Mar 2024 11:22:11 -0500 Subject: [PATCH 5/6] [npm#5280] Allow --no-save to prevent other save flags from overwriting save --- workspaces/config/lib/index.js | 3 ++- workspaces/config/test/index.js | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index 158bef8b8c8bf..f3cb891c95aeb 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -235,7 +235,8 @@ class Config { // Only set 'save' to true if a saveType has been specified // and if 'save' is false due to non-default config - if (!this.isDefault('save') && this.#flatOptions.saveType) { + if (!this.isDefault('save') && this.#flatOptions.saveType + && !this.argv?.includes('--no-save')) { this.#flatOptions.save = true } this.#flatOptions.nodeBin = this.execPath diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index 88b238967e610..231b6448ba930 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -944,9 +944,9 @@ t.test('manages the save flag when flat is retrieved', t => { const c = await buildConfig(['--no-save'], true) t.equal(c.flat.save, false) }) - t.test('overwrites save if --no-save is present and also a save flag', async t => { - const c = await buildConfig(['--save-prod', '--no-save'], false) - t.equal(c.flat.save, true) + t.test('oes not overwrite save --no-save is present in addition to save flag', async t => { + const c = await buildConfig(['--save-prod', '--no-save'], true) + t.equal(c.flat.save, false) }) t.end() }) From 8e193949a530804fc19c84254357da92c955b367 Mon Sep 17 00:00:00 2001 From: Michael Ficocelli Date: Thu, 14 Mar 2024 21:35:24 -0400 Subject: [PATCH 6/6] [npm#5280] remove --no-save handling, left as non-supported --- workspaces/config/lib/index.js | 3 +-- workspaces/config/test/index.js | 4 ---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index f3cb891c95aeb..158bef8b8c8bf 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -235,8 +235,7 @@ class Config { // Only set 'save' to true if a saveType has been specified // and if 'save' is false due to non-default config - if (!this.isDefault('save') && this.#flatOptions.saveType - && !this.argv?.includes('--no-save')) { + if (!this.isDefault('save') && this.#flatOptions.saveType) { this.#flatOptions.save = true } this.#flatOptions.nodeBin = this.execPath diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index 231b6448ba930..c5ac7e787ea78 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -944,10 +944,6 @@ t.test('manages the save flag when flat is retrieved', t => { const c = await buildConfig(['--no-save'], true) t.equal(c.flat.save, false) }) - t.test('oes not overwrite save --no-save is present in addition to save flag', async t => { - const c = await buildConfig(['--save-prod', '--no-save'], true) - t.equal(c.flat.save, false) - }) t.end() })