fix(ci): rm workspace node_modules

lib/commands/ci.js

@@ -1,6 +1,7 @@
 const reifyFinish = require('../utils/reify-finish.js')
 const runScript = require('@npmcli/run-script')
 const fs = require('fs/promises')
+const path = require('path')
 const { log, time } = require('proc-log')
 const validateLockfile = require('../utils/validate-lockfile.js')
 const ArboristWorkspaceCmd = require('../arborist-cmd.js')
@@ -86,11 +87,11 @@ class CI extends ArboristWorkspaceCmd {
       // tree and validated the lockfile
       await time.start('npm-ci:rm', async () => {
         return await Promise.all([...workspacePaths.values()].map(async modulePath => {
-          const path = `${modulePath}/node_modules`
+          const fullPath = path.join(modulePath, 'node_modules')
           // get the list of entries so we can skip the glob for performance
-          const entries = await fs.readdir(path, null).catch(() => [])
-          return Promise.all(entries.map(f => {
-            return fs.rm(`${path}/${f}`, { force: true, recursive: true })
+          const entries = await fs.readdir(fullPath, null).catch(() => [])
+          return Promise.all(entries.map(folder => {
+            return fs.rm(path.join(fullPath, folder), { force: true, recursive: true })
           }))
         }))
       })

test/fixtures/mock-npm.js

@@ -316,104 +316,112 @@ const loadNpmWithRegistry = async (t, opts) => {
     )
   }
 
-  const assert = { fileShouldExist, fileShouldNotExist, packageVersionMatches }
-
-  return { registry, assert, ...mock }
-}
-
-/** breaks down a spec "abbrev@1.1.1" into different parts for mocking */
-function dep (spec, opt) {
-  const [name, version = '1.0.0'] = spec.split('@')
-  const lockPath = !opt.hoist && opt.parent ? `${opt.parent}/` : ''
-  const { definition } = opt
-
-  const depsMap = definition ? Object.entries(definition).map(([s, o]) => {
-    return dep(s, { ...o, parent: name })
-  }) : []
-  const dependencies = Object.assign({}, ...depsMap.map(d => d.asDependency))
-
-  const asPackageJSON = JSON.stringify({
-    name, version, ...(Object.keys(dependencies).length ? { dependencies } : {}),
-  })
-
-  const asDependency = {
-    [name]: version,
+  const packageInstalled = (target) => {
+    const spec = path.basename(target)
+    const dirname = path.dirname(target)
+    const [name, version = '1.0.0'] = spec.split('@')
+    assert.fileShouldNotExist(`${dirname}/${name}/${name}@${version}.txt`)
+    assert.packageVersionMatches(`${dirname}/${name}/package.json`, version)
+    assert.fileShouldExist(`${dirname}/${name}/index.js`)
   }
 
-  const asPackageLock = {
-    [`${lockPath}node_modules/${name}`]: {
-      version,
-      resolved: `https://registry.npmjs.org/${name}/-/${name}-${version}.tgz`,
-    },
+  const packageMissing = (target) => {
+    const spec = path.basename(target)
+    const dirname = path.dirname(target)
+    const [name, version = '1.0.0'] = spec.split('@')
+    assert.fileShouldNotExist(`${dirname}/${name}/${name}@${version}.txt`)
+    assert.fileShouldNotExist(`${dirname}/${name}/package.json`)
+    assert.fileShouldNotExist(`${dirname}/${name}/index.js`)
   }
-  const asPackage = {
-    'package.json': asPackageJSON,
-    'index.js': 'module.exports = "hello world"',
-  }
-
-  const asTarball = [`${name}@${version}`, asPackage]
 
-  const asDirtyModule = {
-    [name]: {
-      [`${name}@${version}.txt`]: '',
-      'package.json': asPackageJSON,
-    },
+  const packageDirty = (target) => {
+    const spec = path.basename(target)
+    const dirname = path.dirname(target)
+    const [name, version = '1.0.0'] = spec.split('@')
+    assert.fileShouldExist(`${dirname}/${name}/${name}@${version}.txt`)
+    assert.packageVersionMatches(`${dirname}/${name}/package.json`, version)
+    assert.fileShouldNotExist(`${dirname}/${name}/index.js`)
   }
 
-  const asLockLink = {
-    [`node_modules/${name}`]: {
-      resolved: `${name}`,
-      link: true,
-    },
+  const assert = {
+    fileShouldExist,
+    fileShouldNotExist,
+    packageVersionMatches,
+    packageInstalled,
+    packageMissing,
+    packageDirty,
   }
 
-  const asDepLock = depsMap.map(d => d.asPackageLock)
-  const asLocalLockEntry = { [name]: { version, dependencies } }
-
-  const asModule = {
-    [name]: {
-      node_modules: Object.assign({}, ...depsMap.map(d => d.hoist ? {} : d.asDirtyModule)),
-      ...asPackage,
-    },
-  }
+  return { registry, assert, ...mock }
+}
 
-  const asLocalizedDirty = lockPath ? {
-    ...asDirtyModule,
-  } : {}
+/** breaks down a spec "abbrev@1.1.1" into different parts for mocking */
+function dependencyDetails (spec, opt = {}) {
+  const [name, version = '1.0.0'] = spec.split('@')
+  const { parent, hoist = true, ws, clean = true } = opt
+  const modulePathPrefix = !hoist && parent ? `${parent}/` : ''
+  const modulePath = `${modulePathPrefix}node_modules/${name}`
+  const resolved = `https://registry.npmjs.org/${name}/-/${name}-${version}.tgz`
+  // deps
+  const wsEntries = Object.entries({ ...ws })
+  const depsMap = wsEntries.map(([s, o]) => dependencyDetails(s, { ...o, parent: name }))
+  const dependencies = Object.assign({}, ...depsMap.map(d => d.packageDependency))
+  const spreadDependencies = depsMap.length ? { dependencies } : {}
+  // package and lock objects
+  const packageDependency = { [name]: version }
+  const packageLockEntry = { [modulePath]: { version, resolved } }
+  const packageLockLink = { [modulePath]: { resolved: name, link: true } }
+  const packageLockLocal = { [name]: { version, dependencies } }
+  // build package.js
+  const packageJSON = { name, version, ...spreadDependencies }
+  const packageJSONString = JSON.stringify(packageJSON)
+  const packageJSONFile = { 'package.json': packageJSONString }
+  // build index.js
+  const indexJSString = 'module.exports = "hello world"'
+  const indexJSFile = { 'index.js': indexJSString }
+  // tarball
+  const packageFiles = { ...packageJSONFile, ...indexJSFile }
+  const nodeModules = Object.assign({}, ...depsMap.map(d => d.hoist ? {} : d.dirtyOrCleanDir))
+  const nodeModulesDir = { node_modules: nodeModules }
+  const packageDir = { [name]: { ...packageFiles, ...nodeModulesDir } }
+  const tarballDir = { [`${name}@${version}`]: packageFiles }
+  // dirty files
+  const dirtyFile = { [`${name}@${version}.txt`]: 'dirty file' }
+  const dirtyFiles = { ...packageJSONFile, ...dirtyFile }
+  const dirtyDir = { [name]: dirtyFiles }
+  const dirtyOrCleanDir = clean ? {} : dirtyDir
 
   return {
-    ...opt,
-    name,
-    version,
-    asTarball,
-    asPackage,
-    asLocalizedDirty,
-    asDirtyModule,
-    asPackageJSON,
-    asPackageLock,
-    asDependency,
-    asModule,
+    packageDependency,
+    hoist,
     depsMap,
-    asLockLink,
-    asDepLock,
-    asLocalLockEntry,
+    dirtyOrCleanDir,
+    tarballDir,
+    packageDir,
+    packageLockEntry,
+    packageLockLink,
+    packageLockLocal,
   }
 }
 
 function workspaceMock (t, opts) {
-  const { clean, workspaces } = { clean: true, ...opts }
-
+  const toObject = [(a, c) => ({ ...a, ...c }), {}]
+  const { workspaces: workspacesDef, ...rest } = { clean: true, ...opts }
+  const workspaces = Object.fromEntries(Object.entries(workspacesDef).map(([name, ws]) => {
+    return [name, Object.fromEntries(Object.entries(ws).map(([wsPackageDep, wsPackageDepOpts]) => {
+      return [wsPackageDep, { ...rest, ...wsPackageDepOpts }]
+    }))]
+  }))
   const root = 'workspace-root'
   const version = '1.0.0'
   const names = Object.keys(workspaces)
-  const ws = Object.entries(workspaces).map(([name, definition]) => dep(name, { definition }))
-  const deps = ws.map(w => w.depsMap).flat()
-  const tarballs = Object.fromEntries(deps.map(flatDep => flatDep.asTarball))
-  const symlinks = Object.fromEntries(names.map((name) => {
-    return [name, t.fixture('symlink', `../${name}`)]
-  }))
-  const hoisted = Object.assign({}, ...deps.filter(d => d.hoist).map(d => d.asDirtyModule))
-  const workspaceFolders = Object.assign({}, ...ws.map(w => w.asModule))
+  const ws = Object.entries(workspaces).map(([name, _ws]) => dependencyDetails(name, { ws: _ws }))
+  const deps = ws.map(({ depsMap }) => depsMap).flat()
+  const tarballs = deps.map(w => w.tarballDir).reduce(...toObject)
+  const symlinks = names
+    .map((name) => ({ [name]: t.fixture('symlink', `../${name}`) })).reduce(...toObject)
+  const hoisted = deps.filter(d => d.hoist).map(w => w.dirtyOrCleanDir).reduce(...toObject)
+  const workspaceFolders = ws.map(w => w.packageDir).reduce(...toObject)
   const packageJSON = { name: root, version, workspaces: names }
   const packageLockJSON = ({
     name: root,
@@ -422,28 +430,22 @@ function workspaceMock (t, opts) {
     requires: true,
     packages: {
       '': { name: root, version, workspaces: names },
-      ...Object.assign({}, ...ws.map(d => d.asLockLink).flat()),
-      ...Object.assign({}, ...ws.map(d => d.asDepLock).flat()),
-      ...Object.assign({}, ...ws.map(d => d.asLocalLockEntry).flat()),
+      ...deps.filter(d => d.hoist).map(d => d.packageLockEntry).reduce(...toObject),
+      ...ws.map(d => d.packageLockEntry).flat().reduce(...toObject),
+      ...ws.map(d => d.packageLockLink).flat().reduce(...toObject),
+      ...ws.map(d => d.packageLockLocal).flat().reduce(...toObject),
+      ...deps.filter(d => !d.hoist).map(d => d.packageLockEntry).reduce(...toObject),
     },
   })
-
   return {
     tarballs,
-    node_modules: clean ? {} : {
+    node_modules: {
       ...hoisted,
       ...symlinks,
     },
     'package-lock.json': JSON.stringify(packageLockJSON),
     'package.json': JSON.stringify(packageJSON),
-    ...Object.fromEntries(Object.entries(workspaceFolders).map(([_key, value]) => {
-      return [_key, Object.fromEntries(Object.entries(value).map(([key, valueTwo]) => {
-        if (key === 'node_modules' && clean) {
-          return [key, {}]
-        }
-        return [key, valueTwo]
-      }))]
-    })),
+    ...workspaceFolders,
   }
 }
 

test/lib/commands/ci.js

@@ -233,7 +233,7 @@ t.test('should remove dirty node_modules with unhoisted workspace module', async
       clean: false,
       workspaces: {
         'workspace-a': {
-          'abbrev@1.1.0': { hoist: true },
+          'abbrev@1.1.0': { },
         },
         'workspace-b': {
           'abbrev@1.1.1': { hoist: false },
@@ -246,15 +246,11 @@ t.test('should remove dirty node_modules with unhoisted workspace module', async
     'abbrev@1.1.1': path.join(npm.prefix, 'tarballs/abbrev@1.1.1'),
   })
   registry.nock.post('/-/npm/v1/security/advisories/bulk').reply(200, {})
+  assert.packageDirty('node_modules/abbrev@1.1.0')
+  assert.packageDirty('workspace-b/node_modules/abbrev@1.1.1')
   await npm.exec('ci', [])
-  // for abbrev@1.1.0
-  assert.fileShouldNotExist('node_modules/abbrev/abbrev@1.1.0.txt')
-  assert.packageVersionMatches('node_modules/abbrev/package.json', '1.1.0')
-  assert.fileShouldExist('node_modules/abbrev/index.js')
-  // for abbrev@1.1.1
-  assert.fileShouldNotExist('workspace-b/node_modules/abbrev/abbrev@1.1.1.txt')
-  assert.packageVersionMatches('workspace-b/node_modules/abbrev/package.json', '1.1.1')
-  assert.fileShouldExist('workspace-b/node_modules/abbrev/index.js')
+  assert.packageInstalled('node_modules/abbrev@1.1.0')
+  assert.packageInstalled('workspace-b/node_modules/abbrev@1.1.1')
 })
 
 t.test('should remove dirty node_modules with hoisted workspace modules', async t => {
@@ -263,10 +259,10 @@ t.test('should remove dirty node_modules with hoisted workspace modules', async
       clean: false,
       workspaces: {
         'workspace-a': {
-          'abbrev@1.1.0': { hoist: true },
+          'abbrev@1.1.0': { },
         },
         'workspace-b': {
-          'lodash@1.1.1': { hoist: true },
+          'lodash@1.1.1': { },
         },
       },
     }),
@@ -276,19 +272,16 @@ t.test('should remove dirty node_modules with hoisted workspace modules', async
     'lodash@1.1.1': path.join(npm.prefix, 'tarballs/lodash@1.1.1'),
   })
   registry.nock.post('/-/npm/v1/security/advisories/bulk').reply(200, {})
+  assert.packageDirty('node_modules/abbrev@1.1.0')
+  assert.packageDirty('node_modules/lodash@1.1.1')
   await npm.exec('ci', [])
-  // for abbrev@1.1.0
-  assert.fileShouldNotExist('node_modules/abbrev/abbrev@1.1.0.txt')
-  assert.packageVersionMatches('node_modules/abbrev/package.json', '1.1.0')
-  assert.fileShouldExist('node_modules/abbrev/index.js')
-  // for lodash@1.1.1
-  assert.fileShouldNotExist('node_modules/lodash/lodash@1.1.1.txt')
-  assert.packageVersionMatches('node_modules/lodash/package.json', '1.1.1')
-  assert.fileShouldExist('node_modules/lodash/index.js')
+  assert.packageInstalled('node_modules/abbrev@1.1.0')
+  assert.packageInstalled('node_modules/lodash@1.1.1')
 })
 
 /** this behaves the same way as install but will remove all workspace node_modules */
 t.test('should use --workspace flag', async t => {
+  t.saveFixture = true
   const { npm, registry, assert } = await loadMockNpm(t, {
     config: {
       workspace: 'workspace-b',
@@ -297,10 +290,10 @@ t.test('should use --workspace flag', async t => {
       clean: false,
       workspaces: {
         'workspace-a': {
-          'abbrev@1.1.0': { hoist: true },
+          'abbrev@1.1.0': { },
         },
         'workspace-b': {
-          'lodash@1.1.1': { hoist: true },
+          'lodash@1.1.1': { },
         },
       },
     }),
@@ -309,13 +302,9 @@ t.test('should use --workspace flag', async t => {
     'lodash@1.1.1': path.join(npm.prefix, 'tarballs/lodash@1.1.1'),
   })
   registry.nock.post('/-/npm/v1/security/advisories/bulk').reply(200, {})
+  assert.packageDirty('node_modules/abbrev@1.1.0')
+  assert.packageDirty('node_modules/lodash@1.1.1')
   await npm.exec('ci', [])
-  // for abbrev@1.1.0
-  assert.fileShouldNotExist('node_modules/abbrev/abbrev@1.1.0.txt')
-  assert.fileShouldNotExist('node_modules/abbrev/package.json')
-  assert.fileShouldNotExist('node_modules/abbrev/index.js')
-  // for lodash@1.1.1
-  assert.fileShouldNotExist('node_modules/lodash/lodash@1.1.1.txt')
-  assert.packageVersionMatches('node_modules/lodash/package.json', '1.1.1')
-  assert.fileShouldExist('node_modules/lodash/index.js')
+  assert.packageMissing('node_modules/abbrev@1.1.0')
+  assert.packageInstalled('node_modules/lodash@1.1.1')
 })