fix: save packages to the correct properties

Close #727

src/api/extendOptions.ts

@@ -60,10 +60,6 @@ export default (opts?: PnpmOptions): StrictPnpmOptions => {
   if (extendedOpts.localRegistry !== DEFAULT_LOCAL_REGISTRY) {
     extendedOpts.localRegistry = expandTilde(extendedOpts.localRegistry, extendedOpts.prefix)
   }
-  if (extendedOpts.save === false && extendedOpts.saveDev === false && extendedOpts.saveOptional === false) {
-    throw new Error('Cannot install with save/saveDev/saveOptional all being equal false')
-  }
-  extendedOpts.save = extendedOpts.save || !extendedOpts.saveDev && !extendedOpts.saveOptional
   if (extendedOpts.userAgent.startsWith('npm/')) {
     extendedOpts.userAgent = `${pnpmPkgJson.name}/${pnpmPkgJson.version} ${extendedOpts.userAgent}`
   }

src/api/uninstall.ts

@@ -44,7 +44,7 @@ export default async function uninstallCmd (pkgsToUninstall: string[], maybeOpts
 }
 
 export async function uninstallInContext (pkgsToUninstall: string[], pkg: Package, ctx: PnpmContext, opts: StrictPnpmOptions) {
-  const saveType = getSaveType(opts)
+  const saveType = getSaveType(opts) || 'dependencies'
   if (saveType) {
     const pkgJsonPath = path.join(ctx.root, 'package.json')
     const pkg = await removeDeps(pkgJsonPath, pkgsToUninstall, saveType)

src/getSaveType.ts

@@ -1,8 +1,11 @@
 import {PnpmOptions} from './types'
 export type DependenciesType = 'dependencies' | 'devDependencies' | 'optionalDependencies'
 
-export default function getSaveType (opts: PnpmOptions): DependenciesType {
+export const dependenciesTypes: DependenciesType[] = ['dependencies', 'devDependencies', 'optionalDependencies']
+
+export default function getSaveType (opts: PnpmOptions): DependenciesType | undefined {
   if (opts.saveDev) return 'devDependencies'
   if (opts.saveOptional) return 'optionalDependencies'
-  return 'dependencies'
+  if (opts.save) return 'dependencies'
+  return undefined
 }

src/save.ts

@@ -1,6 +1,6 @@
 import loadJsonFile = require('load-json-file')
 import writePkg = require('write-pkg')
-import {DependenciesType} from './getSaveType'
+import {DependenciesType, dependenciesTypes} from './getSaveType'
 import {Package} from './types'
 import {PackageSpec} from './resolve'
 
@@ -10,15 +10,33 @@ export default async function save (
     name: string,
     saveSpec: string,
   })[],
-  saveType: DependenciesType
+  saveType?: DependenciesType
 ): Promise<Package> {
   // Read the latest version of package.json to avoid accidental overwriting
   const packageJson = await loadJsonFile(pkgJsonPath)
-  packageJson[saveType] = packageJson[saveType] || {}
-  packageSpecs.forEach(dependency => {
-    packageJson[saveType][dependency.name] = dependency.saveSpec
-  })
+  if (saveType) {
+    packageJson[saveType] = packageJson[saveType] || {}
+    packageSpecs.forEach(dependency => {
+      packageJson[saveType][dependency.name] = dependency.saveSpec
+      dependenciesTypes.filter(deptype => deptype !== saveType).forEach(deptype => {
+        if (packageJson[deptype]) {
+          delete packageJson[deptype][dependency.name]
+        }
+      })
+    })
+  } else {
+    packageSpecs.forEach(dependency => {
+      const usedDepType = guessDependencyType(dependency.name, packageJson) || 'dependencies'
+      packageJson[usedDepType] = packageJson[usedDepType] || {}
+      packageJson[usedDepType][dependency.name] = dependency.saveSpec
+    })
+  }
 
   await writePkg(pkgJsonPath, packageJson)
   return packageJson
 }
+
+function guessDependencyType (depName: string, pkg: Package): DependenciesType | undefined {
+  return dependenciesTypes
+    .find(deptype => Boolean(pkg[deptype] && pkg[deptype]![depName]))
+}

test/api.ts

@@ -12,7 +12,11 @@ test('API', t => {
   t.end()
 })
 
-test('install fails when all saving types are false', async t => {
+// TODO: some sort of this validation might need to exist
+// maybe a new property should be introduced
+// this seems illogical as even though all save types are false,
+// the dependency will be saved
+test.skip('install fails when all saving types are false', async (t: test.Test) => {
   try {
     await pnpm.install({save: false, saveDev: false, saveOptional: false})
     t.fail('installation should have failed')

test/install/index.ts

@@ -7,3 +7,4 @@ import './fromRepo'
 import './peerDependencies'
 import './auth'
 import './local'
+import './updatingPkgJson'

test/install/misc.ts

@@ -330,47 +330,6 @@ test('shrinkwrap compatibility', async function (t) {
   })
 })
 
-test('save to package.json (rimraf@2.5.1)', async function (t) {
-  const project = prepare(t)
-  await installPkgs(['rimraf@2.5.1'], testDefaults({ save: true }))
-
-  const m = project.requireModule('rimraf')
-  t.ok(typeof m === 'function', 'rimraf() is available')
-
-  const pkgJson = await readPkg()
-  t.deepEqual(pkgJson.dependencies, {rimraf: '^2.5.1'}, 'rimraf has been added to dependencies')
-})
-
-test('saveDev scoped module to package.json (@rstacruz/tap-spec)', async function (t) {
-  const project = prepare(t)
-  await installPkgs(['@rstacruz/tap-spec'], testDefaults({ saveDev: true }))
-
-  const m = project.requireModule('@rstacruz/tap-spec')
-  t.ok(typeof m === 'function', 'tapSpec() is available')
-
-  const pkgJson = await readPkg()
-  t.deepEqual(pkgJson.devDependencies, { '@rstacruz/tap-spec': '^4.1.1' }, 'tap-spec has been added to devDependencies')
-})
-
-test('multiple save to package.json with `exact` versions (@rstacruz/tap-spec & rimraf@2.5.1) (in sorted order)', async function (t) {
-  const project = prepare(t)
-  await installPkgs(['rimraf@2.5.1', '@rstacruz/tap-spec@latest'], testDefaults({ save: true, saveExact: true }))
-
-  const m1 = project.requireModule('@rstacruz/tap-spec')
-  t.ok(typeof m1 === 'function', 'tapSpec() is available')
-
-  const m2 = project.requireModule('rimraf')
-  t.ok(typeof m2 === 'function', 'rimraf() is available')
-
-  const pkgJson = await readPkg()
-  const expectedDeps = {
-    '@rstacruz/tap-spec': '4.1.1',
-    rimraf: '2.5.1'
-  }
-  t.deepEqual(pkgJson.dependencies, expectedDeps, 'tap-spec and rimraf have been added to dependencies')
-  t.deepEqual(Object.keys(pkgJson.dependencies), Object.keys(expectedDeps), 'tap-spec and rimraf have been added to dependencies in sorted order')
-})
-
 test('production install (with --production flag)', async function (t) {
   const project = prepare(t, basicPackageJson)
 

test/install/updatingPkgJson.ts

@@ -0,0 +1,142 @@
+import tape = require('tape')
+import promisifyTape from 'tape-promise'
+import readPkg = require('read-pkg')
+import {
+  prepare,
+  addDistTag,
+  testDefaults,
+} from '../utils'
+import {installPkgs} from '../../src'
+
+const test = promisifyTape(tape)
+
+test('save to package.json (rimraf@2.5.1)', async function (t) {
+  const project = prepare(t)
+  await installPkgs(['rimraf@2.5.1'], testDefaults({ save: true }))
+
+  const m = project.requireModule('rimraf')
+  t.ok(typeof m === 'function', 'rimraf() is available')
+
+  const pkgJson = await readPkg()
+  t.deepEqual(pkgJson.dependencies, {rimraf: '^2.5.1'}, 'rimraf has been added to dependencies')
+})
+
+test('saveDev scoped module to package.json (@rstacruz/tap-spec)', async function (t) {
+  const project = prepare(t)
+  await installPkgs(['@rstacruz/tap-spec'], testDefaults({ saveDev: true }))
+
+  const m = project.requireModule('@rstacruz/tap-spec')
+  t.ok(typeof m === 'function', 'tapSpec() is available')
+
+  const pkgJson = await readPkg()
+  t.deepEqual(pkgJson.devDependencies, { '@rstacruz/tap-spec': '^4.1.1' }, 'tap-spec has been added to devDependencies')
+})
+
+test('dependency should not be added to package.json if it is already there', async function (t: tape.Test) {
+  await addDistTag('foo', '100.0.0', 'latest')
+  await addDistTag('bar', '100.0.0', 'latest')
+
+  const project = prepare(t, {
+    devDependencies: {
+      foo: '^100.0.0',
+    },
+    optionalDependencies: {
+      bar: '^100.0.0',
+    },
+  })
+  await installPkgs(['foo', 'bar'], testDefaults())
+
+  const pkgJson = await readPkg({normalize: false})
+  t.deepEqual(pkgJson, {
+    name: 'project',
+    version: '0.0.0',
+    devDependencies: {
+      foo: '^100.0.0',
+    },
+    optionalDependencies: {
+      bar: '^100.0.0',
+    },
+  }, 'package.json was not changed')
+})
+
+test('dependencies should be updated in the fields where they already are', async function (t: tape.Test) {
+  await addDistTag('foo', '100.1.0', 'latest')
+  await addDistTag('bar', '100.1.0', 'latest')
+
+  const project = prepare(t, {
+    devDependencies: {
+      foo: '^100.0.0',
+    },
+    optionalDependencies: {
+      bar: '^100.0.0',
+    },
+  })
+  await installPkgs(['foo@latest', 'bar@latest'], testDefaults())
+
+  const pkgJson = await readPkg({normalize: false})
+  t.deepEqual(pkgJson, {
+    name: 'project',
+    version: '0.0.0',
+    devDependencies: {
+      foo: '^100.1.0',
+    },
+    optionalDependencies: {
+      bar: '^100.1.0',
+    },
+  }, 'package.json updated dependencies in the correct properties')
+})
+
+test('dependency should be removed from the old field when installing it as a different type of dependency', async function (t: tape.Test) {
+  await addDistTag('foo', '100.0.0', 'latest')
+  await addDistTag('bar', '100.0.0', 'latest')
+  await addDistTag('qar', '100.0.0', 'latest')
+
+  const project = prepare(t, {
+    dependencies: {
+      foo: '^100.0.0',
+    },
+    devDependencies: {
+      bar: '^100.0.0',
+    },
+    optionalDependencies: {
+      qar: '^100.0.0',
+    },
+  })
+  await installPkgs(['foo'], testDefaults({saveOptional: true}))
+  await installPkgs(['bar'], testDefaults({save: true}))
+  await installPkgs(['qar'], testDefaults({saveDev: true}))
+
+  const pkgJson = await readPkg({normalize: false})
+  t.deepEqual(pkgJson, {
+    name: 'project',
+    version: '0.0.0',
+    dependencies: {
+      bar: '^100.0.0',
+    },
+    devDependencies: {
+      qar: '^100.0.0',
+    },
+    optionalDependencies: {
+      foo: '^100.0.0',
+    },
+  }, 'dependencies moved around correctly')
+})
+
+test('multiple save to package.json with `exact` versions (@rstacruz/tap-spec & rimraf@2.5.1) (in sorted order)', async function (t: tape.Test) {
+  const project = prepare(t)
+  await installPkgs(['rimraf@2.5.1', '@rstacruz/tap-spec@latest'], testDefaults({ save: true, saveExact: true }))
+
+  const m1 = project.requireModule('@rstacruz/tap-spec')
+  t.ok(typeof m1 === 'function', 'tapSpec() is available')
+
+  const m2 = project.requireModule('rimraf')
+  t.ok(typeof m2 === 'function', 'rimraf() is available')
+
+  const pkgJson = await readPkg()
+  const expectedDeps = {
+    '@rstacruz/tap-spec': '4.1.1',
+    rimraf: '2.5.1'
+  }
+  t.deepEqual(pkgJson.dependencies, expectedDeps, 'tap-spec and rimraf have been added to dependencies')
+  t.deepEqual(Object.keys(pkgJson.dependencies), Object.keys(expectedDeps), 'tap-spec and rimraf have been added to dependencies in sorted order')
+})

test/utils/prepare.ts

@@ -23,7 +23,7 @@ export default function prepare (t: Test, pkg?: Object) {
   const dirname = dirNumber.toString()
   const pkgTmpPath = path.join(tmpPath, dirname, 'project')
   mkdirp.sync(pkgTmpPath)
-  const json = JSON.stringify(Object.assign({name: 'foo', version: '0.0.0'}, pkg), null, 2)
+  const json = JSON.stringify(Object.assign({name: 'project', version: '0.0.0'}, pkg), null, 2)
   fs.writeFileSync(path.join(pkgTmpPath, 'package.json'), json, 'utf-8')
   process.chdir(pkgTmpPath)
   t.pass(`create testing package ${dirname}`)