Skip to content

Commit

Permalink
fix: addressed code review
Browse files Browse the repository at this point in the history
- added more tests scenarios
- fixed some typos
  • Loading branch information
ruyadorno committed Jan 20, 2022
1 parent 3bb7bac commit b50701a
Show file tree
Hide file tree
Showing 15 changed files with 756 additions and 208 deletions.
8 changes: 4 additions & 4 deletions docs/content/using-npm/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -1326,16 +1326,16 @@ The base URL of the npm registry.

#### `save`

* Default: true unless when using `npm update` or `npm dedupe` where it
* Default: `true` unless when using `npm update` or `npm dedupe` where it
defaults to `false`
* Type: Boolean

Save installed packages to a package.json file as dependencies.
Save installed packages to a `package.json` file as dependencies.

When used with the `npm rm` command, removes the dependency from
package.json.
`package.json`.

Will also prevent writing to package-lock.json if set to `false`.
Will also prevent writing to `package-lock.json` if set to `false`.

<!-- automatically generated, do not edit manually -->
<!-- see lib/utils/config/definitions.js -->
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/update.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class Update extends ArboristWorkspaceCmd {
? global
: this.npm.prefix

// In the context of `npm upgrade` the save
// In the context of `npm update` the save
// config value should default to `false`
const save = this.npm.config.isDefault('save')
? false
Expand Down
8 changes: 4 additions & 4 deletions lib/utils/config/definitions.js
Original file line number Diff line number Diff line change
Expand Up @@ -1583,18 +1583,18 @@ define('registry', {

define('save', {
default: true,
defaultDescription: `true unless when using \`npm update\` or
defaultDescription: `\`true\` unless when using \`npm update\` or
\`npm dedupe\` where it defaults to \`false\``,
usage: '-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer',
type: Boolean,
short: 'S',
description: `
Save installed packages to a package.json file as dependencies.
Save installed packages to a \`package.json\` file as dependencies.
When used with the \`npm rm\` command, removes the dependency from
package.json.
\`package.json\`.
Will also prevent writing to package-lock.json if set to \`false\`.
Will also prevent writing to \`package-lock.json\` if set to \`false\`.
`,
flatten,
})
Expand Down
55 changes: 48 additions & 7 deletions smoke-tests/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -268,14 +268,55 @@ t.test('npm pkg', async t => {
)
})

t.test('npm update --no-save --no-package-lock', async t => {
// setup, manually reset dep value
await exec(`${npmBin} pkg set dependencies.abbrev==1.0.4`)
await exec(`${npmBin} install`)
await exec(`${npmBin} pkg set dependencies.abbrev=^1.0.4`)

const cmd = `${npmBin} update --no-save --no-package-lock`
await exec(cmd)

t.equal(
JSON.parse(readFile('package.json')).dependencies.abbrev,
'^1.0.4',
'should have expected update --no-save --no-package-lock package.json result'
)
t.matchSnapshot(
JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'],
'1.0.4',
'should have expected update --no-save --no-package-lock lockfile result'
)
})

t.test('npm update --no-save', async t => {
const cmd = `${npmBin} update --no-save`
await exec(cmd)

t.matchSnapshot(
JSON.parse(readFile('package.json')).dependencies.abbrev,
'^1.0.4',
'should have expected update --no-save package.json result'
)
t.matchSnapshot(
JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'],
'1.1.1',
'should have expected update --no-save lockfile result'
)
})

t.test('npm update --save', async t => {
const cmd = `${npmBin} update --save`
const cmdRes = await exec(cmd)
await exec(cmd)

t.matchSnapshot(cmdRes.replace(/in.*s/, ''),
'should have expected update --save reify output')
t.matchSnapshot(readFile('package.json'),
'should have expected update --save package.json result')
t.matchSnapshot(readFile('package-lock.json'),
'should have expected update --save lockfile result')
t.matchSnapshot(
JSON.parse(readFile('package.json')).dependencies.abbrev,
'^1.1.1',
'should have expected update --save package.json result'
)
t.matchSnapshot(
JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'],
'1.1.1',
'should have expected update --save lockfile result'
)
})
70 changes: 22 additions & 48 deletions tap-snapshots/smoke-tests/index.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -666,62 +666,36 @@ removed 1 package
`

exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save lockfile result 1`] = `
{
"name": "project",
"version": "1.0.0",
"lockfileVersion": 2,
"requires": true,
"packages": {
"": {
"name": "project",
"version": "1.0.0",
"license": "ISC",
"dependencies": {
"abbrev": "^1.1.1"
}
},
"node_modules/abbrev": {
"version": "1.1.1",
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
"integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
}
},
"dependencies": {
"abbrev": {
"version": "1.1.1",
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
"integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
}
}
exports[`smoke-tests/index.js TAP npm update --no-save --no-package-lock > 1.0.4 1`] = `
Object {
"integrity": "sha1-vVWuXkE7oXIu5Mq6H26hBBSlns0=",
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.0.4.tgz",
"version": "1.0.4",
}
`

exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save package.json result 1`] = `
{
"name": "project",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo /"Error: no test specified/" && exit 1",
"hello": "echo Hello"
},
"keywords": [],
"author": "",
"license": "ISC",
"dependencies": {
"abbrev": "^1.1.1"
}
exports[`smoke-tests/index.js TAP npm update --no-save > 1.1.1 1`] = `
Object {
"integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==",
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
"version": "1.1.1",
}
`

exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save reify output 1`] = `
exports[`smoke-tests/index.js TAP npm update --no-save > ^1.0.4 1`] = `
^1.0.4
`

up to date
exports[`smoke-tests/index.js TAP npm update --save > 1.1.1 1`] = `
Object {
"integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==",
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
"version": "1.1.1",
}
`

exports[`smoke-tests/index.js TAP npm update --save > ^1.1.1 1`] = `
^1.1.1
`

exports[`smoke-tests/index.js TAP npm update dep > should have expected update lockfile result 1`] = `
Expand Down
8 changes: 4 additions & 4 deletions tap-snapshots/test/lib/utils/config/definitions.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1406,16 +1406,16 @@ The base URL of the npm registry.
exports[`test/lib/utils/config/definitions.js TAP > config description for save 1`] = `
#### \`save\`
* Default: true unless when using \`npm update\` or \`npm dedupe\` where it
* Default: \`true\` unless when using \`npm update\` or \`npm dedupe\` where it
defaults to \`false\`
* Type: Boolean
Save installed packages to a package.json file as dependencies.
Save installed packages to a \`package.json\` file as dependencies.
When used with the \`npm rm\` command, removes the dependency from
package.json.
\`package.json\`.
Will also prevent writing to package-lock.json if set to \`false\`.
Will also prevent writing to \`package-lock.json\` if set to \`false\`.
`

exports[`test/lib/utils/config/definitions.js TAP > config description for save-bundle 1`] = `
Expand Down
8 changes: 4 additions & 4 deletions tap-snapshots/test/lib/utils/config/describe-all.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1200,16 +1200,16 @@ The base URL of the npm registry.
#### \`save\`
* Default: true unless when using \`npm update\` or \`npm dedupe\` where it
* Default: \`true\` unless when using \`npm update\` or \`npm dedupe\` where it
defaults to \`false\`
* Type: Boolean
Save installed packages to a package.json file as dependencies.
Save installed packages to a \`package.json\` file as dependencies.
When used with the \`npm rm\` command, removes the dependency from
package.json.
\`package.json\`.
Will also prevent writing to package-lock.json if set to \`false\`.
Will also prevent writing to \`package-lock.json\` if set to \`false\`.
<!-- automatically generated, do not edit manually -->
<!-- see lib/utils/config/definitions.js -->
Expand Down
16 changes: 10 additions & 6 deletions workspaces/arborist/lib/arborist/reify.js
Original file line number Diff line number Diff line change
Expand Up @@ -1142,21 +1142,25 @@ module.exports = cls => class Reifier extends cls {
// for install failures. Those still end up in the shrinkwrap, so we
// save it first, then prune out the optional trash, and then return it.

const skipSave = options.save === false
const save = !skipSave
const save = !(options.save === false)

// we check for updates in order to make sure we run save ideal tree
// even though save=false since we want `npm update` to be able to
// write to package-lock files by default
const hasUpdates = this[_updateAll] || this[_updateNames].length

// we're going to completely skip save ideal tree in case of a global or
// dry-run install and also if the save option is set to false, EXCEPT for
// update since the expected behavior for npm7+ is for update to
// NOT save to package.json, we make that exception since we still want
// saveIdealTree to be able to write the lockfile
const skipSaveIdealTree =
(skipSave && !hasUpdates)
// saveIdealTree to be able to write the lockfile by default.
const saveIdealTree = !(
(!save && !hasUpdates)
|| this[_global]
|| this[_dryRun]
)

if (skipSaveIdealTree) {
if (!saveIdealTree) {
return false
}

Expand Down
Loading

0 comments on commit b50701a

Please sign in to comment.